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

Support commonjs import standard for node vs web #838

Closed
fundthmcalculus opened this issue Jul 5, 2022 · 14 comments
Closed

Support commonjs import standard for node vs web #838

fundthmcalculus opened this issue Jul 5, 2022 · 14 comments
Assignees
Labels
discussion Issue is a discussion
Milestone

Comments

@fundthmcalculus
Copy link
Member

We have a request to switch back from es2020 to commonjs module support. This would enable older versions of node and eliminate the requirement for type: module, while requiring us to have separate node and web packages. We combined the packages so that we didn't have code duplication between node and web packages. That being said, we have automatic template code generation now, so the code duplication is less of an issue. Thoughts on ways forward:

  1. Keep as is, which also has implications for support in web (older versions of webpack / craco / etc)
  2. Split into two packages in code (the way we used to do things): gives us complete customization at the cost of code duplication
  3. Common code, two copies of tsconfig.json, and change the module format on npm publish. (testing issues?)
  4. Lerna is unsupported, so the older method for mixing packages isn't really an option anymore.

If we pick options 2 or 3, we should probably do the same for okapi to eliminate any issues on that front. Okapi doesn't have as many dependencies, so we aren't creating issues with webpack and polyfills there.

@fundthmcalculus fundthmcalculus added the discussion Issue is a discussion label Jul 5, 2022
@fundthmcalculus fundthmcalculus self-assigned this Jul 5, 2022
@fundthmcalculus fundthmcalculus added this to the SDK 1.7.0 milestone Jul 5, 2022
@MichaelEdwardBlack
Copy link
Contributor

For completely biased reasons I like the idea of option 2 and am curious about option 3. However, I also understand it's a selfish/biased reason for me because it just helps me with create-react-app but I'm also trying to do some research so that I can make guides in helping customers with React if we do stay with option 1.

@sethjback
Copy link
Contributor

sethjback commented Jul 5, 2022

I would highly recommend we do not drop commonjs support - this is actually one of the reasons why we can't update our current json-ld codebase - the library from digital bazaar moved to ESM but other dependencies from Matter still use CJS. I am not a javascript expert, but I found this article helpful when digging into the json-ld stuff, especially his recommendation about the possibility of using a thin EMS wrapper for CJS exports:

https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

@fundthmcalculus
Copy link
Member Author

And for reference, I have tested umd, it doesn't work with older versions of react/webpack etc @MichaelEdwardBlack .

@sethjback , it sounds like the answer is, "we must support commonjs directly". I agree. Let's see how we can do that with a minimum of duplicated effort.

@fundthmcalculus
Copy link
Member Author

From @janpieterz : I do really like having 1 package though, for consumers that’s so much easier, any tiny friction should, imho, be avoided. Wouldn’t having the Seth Back link be functional, keeping the code separate but allow 0 friction?

Hmmm, this is an interesting approach! This would solve the node side of the problem. It doesn't solve the web side of the problem, wherein the dependencies cause issues with webpack and polyfills.

It sounds like the right answer to this problem is:

  1. Publish two packages: @trinsic-id/sdk-node and @trinsic-id/sdk-web with only the relevant dependencies.
  2. For sdk-node use cjs by default, and if necessary, add an esm wrapper around that @sethjback link.
  3. Ideally, we can keep the source code common between them, and alter the package.json and tsconfig.json as needed.

@sethjback @janpieterz @MichaelEdwardBlack what say you?

@janpieterz
Copy link
Contributor

janpieterz commented Jul 5, 2022

It's almost the only solution. Is this feasible to reach before GA, with the other work in the sprint?

@fundthmcalculus
Copy link
Member Author

It's almost the only solution. Is this feasible to reach before GA, with the other work in the sprint?

I'm pretty sure I can pull it off. It's affecting a customer, so it's sort of required pre-GA.

@sethjback
Copy link
Contributor

Am I mis-reading this from the article? Sounds like he is suggesting a single CJS library with:

Put the ESM wrapper in an esm subdirectory, alongside a one-line package.json file that says {"type": "module"}. (You could rename your wrapper file to .mjs instead, and that will work fine in Node 14, but some tools don’t work well with .mjs files, so I prefer to use a subdirectory.)

Doesn't that solve the issue without a split codebase (i.e. two packages)? You just document including that esm subdirectory / package.json for your node users, everyone else uses the regular codebase.

Again - not a javascript expert here...

@fundthmcalculus
Copy link
Member Author

Doesn't that solve the issue without a split codebase (i.e. two packages)? You just document including that esm subdirectory / package.json for your node users, everyone else uses the regular codebase.

That prevents splitting trinsici-id/sdk-node/cjs and trinsic-id/sdk-node/esm. We have to split trinsic-id/sdk-node and trinsic-id/sdk-web due to node dependencies causing issues with webpack polyfills etc.

Again - not a javascript expert here...

Me neither, lol. We need a better front-end guy than me to own this type of thing.

@janpieterz
Copy link
Contributor

@fundthmcalculus Are the node dependency issues related to es2020? Like, was this an issue before we switched to es2020? Seems like we have had the dependency for a while.

@fundthmcalculus
Copy link
Member Author

We have to use es2020 to get await import, older ESM doesn't fix CommonJS.

@janpieterz
Copy link
Contributor

Ah, I don't know enough.
I'd love to understand it more, Gather in an hour?

@fundthmcalculus
Copy link
Member Author

Ah, I don't know enough. I'd love to understand it more, Gather in an hour?

That sounds like a plan!

@janpieterz
Copy link
Contributor

janpieterz commented Jul 6, 2022

We spoke, we’ll aim to release a single package, commonjs based. Scott will see if using grpc-nice-web will eliminate the need for multiple packages.
If we can’t reach that today we’ll just republish before tonight as-is with commonjs target (without excluding node dependencies) which will unblock our customer.

@fundthmcalculus
Copy link
Member Author

End result: we need two okapi modules: @trinsic/okapi-node and @trinsic/okapi-web due to WASM initialization issues. We can import both back into the SDK and use the browser field to remap from node to web. Verified independently with trinsic-id/sdk-examples#56 (React Wallet sample app and node samples).

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

No branches or pull requests

4 participants