Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(modeling): V2 api compatibility shim #1323

Open
wants to merge 1 commit into
base: V3
Choose a base branch
from

Conversation

platypii
Copy link
Contributor

This PR adds a "V2 compatibility shim" to V3.

In PR #1294 we flattened the JSCAD package heirarchy so that functions like union and extrudeLinear are now in the top level package object:

-import { booleans } from '@jscad/modeling'
-const { union } = boolean
+import { union } from '@jscad/modeling'

But what about all the hundreds of designs out there for V2? They will all break, annoyingly, in the migration to V3.

This PR keeps the flattened structure for V3 but ALSO exports a similar structure to V2. This is not 100% api compatible, due to some fundamental changes in V3, but it is pretty close, and I tried a couple of my designs from V2 against this, and they actually all just-worked!

Feedback welcome, but I think this would make the transition from V2 to V3 much smoother for users of JSCAD.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

@platypii platypii requested review from hrgdavor and z3dev February 17, 2024 16:47
Copy link
Contributor

@hrgdavor hrgdavor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will help with transition to V3, and I do not see a negative impact

@z3dev
Copy link
Member

z3dev commented Feb 26, 2024

I kind of like it and kind of don't, but as @hrgdavor said... these changes help in the transition from V2 to V3.

But is it enough? Would a full V2 compatible shim be better?

@platypii
Copy link
Contributor Author

Would a full V2 compatible shim be better?

This is mostly a full compatibility shim, I tried to make it as complete as I reasonably could. Here is a list of some of the known differences between V2 and this V3-shim:

  • Subtly different behavior on rectangularExtrude
  • Certain operations might fail due to non-manifold geometries: in V2, geom2 was defined by a set of edges, now it is closed loops of points, so it is not possible to represent non-manifold 2D geometries in V3... this leads to occasional discrepancies. Same with slice.
  • Changes to vectorChar and vectorText api
  • Some parameter names changed to camelCase

There are probably some other discrepancies that I missed. But overall, I think these are minor. I am open to suggestions on where to make this shim more api-compatible, but I think this is a pretty good first attempt.

@z3dev
Copy link
Member

z3dev commented Feb 27, 2024

I get it but a little skeptical.

To use the shim requires changes to the imports. So, why not just make a V2 fully compatible shim, which will also requires changes to imports?

I'm not sure which is better.

V2 was criticized by those that like the 'object' chaining. But it provided a new core set of functionality, which surpassed V1 in many ways. Hopefully, V3 will also do the same.

I think the shim is fine, but really think it should be V2 as much as possible. Is it possible?

@platypii
Copy link
Contributor Author

To use the shim requires changes to the imports.

What do you mean? By adding this shim, users do NOT need to change imports from V2 designs. With the shim, this will still work:

const jscad = require('@jscad/modeling')
const { cube } = jscad.primitives

const main = () => cube({ size: 8 })

module.exports = { main }

But so will the more modern V3 syntax:

import { cube } from '@jscad/modeling'
export const main = () => cube({ size: 8 })

This seems like the best of both worlds. We can deprecate the old syntax, and remove the shim in V4?

@z3dev
Copy link
Member

z3dev commented Mar 26, 2024

So there are several things being discussed here...

  1. transcompiling CommonJS to ES5
  2. adding the V2 namespaces back to V3
  3. adding full or partial V2 compatibility
  4. support by the CLI

For transcompiling, this is already supporting in the JSCADUI (Nice!).

Adding the V2 namespaces back seems reasonable, as that will ease the transition of designs from V2 to V3.

As for the full V2 compatibility, I think this should be possible but some special functions have to be written and tested. This compatibility shim could be part of this package, but it would have to be imported specifically by designs. And the compatibility shim might be better as a separate package, again possible.

Also, I think @hrgdavor is now looking at some ways to identify V1 vs V2 vs V3 designs. It would be cool if this could be automatic. But maybe a design can select the API version in a comment, e.g. 'V2'.

Finally, the CLI will become another problem. The CLI may need a total re-write to support transcompiling, different versions of the API, etc.

@z3dev
Copy link
Member

z3dev commented Mar 26, 2024

@platypii can we focus these changes on just point # 2, adding the V2 namespaces back to V3? it's shouldn't be that different.

then we can decide where to put the V2 compatibly shim, and make it 100% compatible, if possible.

@hrgdavor
Copy link
Contributor

  1. transcompiling CommonJS to ES5

It is actually the oposite :) ... it is impossible as far as I looked, to execute ES5 modules the way we need to for jscad worker. ES5 import statements are converted ro require calls so a properly configured require function can be injected. The injected require must have the context what the current path is, so it can resolve relative imports.

We can make a reasonable effort to recognize V1,V2,V3 but we will need to provide a comment with some kind of annotation

  • V1 - one simple way to recognize is for d&d file name ends with .jscad
  • V3 - if using import statements it is likely V3
  • V2 - when using require it is not obvious if it is V2 or V3, annotation would be needed.

some errors could be a giveaway if wrong version is recognized

@z3dev
Copy link
Member

z3dev commented Mar 26, 2024

It is actually the oposite :) ... it is impossible as far as I looked, to execute ES5 modules the way we need to for jscad worker. ES5 import statements are converted ro require calls so a properly configured require function can be injected. The injected require must have the context what the current path is, so it can resolve relative imports.

Wow! That's really a surprise. So, the dynamic imports really don't help? Big shock!

We can make a reasonable effort to recognize V1,V2,V3 but we will need to provide a comment with some kind of annotation

This is probably the best solution, as this could be the standard going forward.

@hrgdavor
Copy link
Contributor

hrgdavor commented Mar 26, 2024

So, the dynamic imports really don't help? Big shock!

I did not try to inject override for dynamic import. Trying to eval a script that uses the import .... from '...' just fails, so at that point it seemed just better to convert to CJS require. Luckily with transpilers working so well for years, this was not a big deal, and with sourcemaps, you can still see proper error reporting and debugging.

also there was compatibility issue for dynamic import in workers last time I looked (if I remember correctly).

@platypii
Copy link
Contributor Author

can we focus these changes on just point # 2, adding the V2 namespaces back to V3? it's shouldn't be that different.
then we can decide where to put the V2 compatibly shim, and make it 100% compatible, if possible.

I am not clear on what you are asking for here? You want the exports in the v2 and v3 namespaces, but none of the compatibility functions? If that is the intent I have to say... why?? You are asking for a world where someone who had a project that used V2 and now upgrades to V3 will have imports that seem to work, but break in mysterious ways. That seems less compatible, and worse than not having the V2-like exports at all!

If you are worried about bloat from adding the functions here? I would agree, except this PR is only 41 lines of code and provides backward compatibility for most jscad V2 designs to upgrade seamlessly to V3. It is a tiny addition, with great benefits. I'm not sure why we wouldn't do this for users? (See #1326 for example of user pain)

@z3dev
Copy link
Member

z3dev commented Mar 28, 2024

can we focus these changes on just point # 2, adding the V2 namespaces back to V3? it's shouldn't be that different.
then we can decide where to put the V2 compatibly shim, and make it 100% compatible, if possible.

I am not clear on what you are asking for here?

In simple terms, a 100% compatible V2 shim. These changes look good initially but there are differences still. And V3 changes are not complete yet. For example, the use of a shared set of attributes for color, etc.

Also, these V2 compatibility changes are being added directly into the V3 API. V2 and V3 in the same package? Why? Will V1 be added as well? There must be another way to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants