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!: Dual-Support ESM & CJS #662

Merged
merged 11 commits into from
Oct 30, 2024
Merged

feat!: Dual-Support ESM & CJS #662

merged 11 commits into from
Oct 30, 2024

Conversation

danielbankhead
Copy link
Contributor

@danielbankhead danielbankhead commented Oct 29, 2024

Notable Changes

  • Created split TS Configs with the default tsconfig.json defaulting to preserve ('esm').
  • Added a basic package.json in build/esm so that all .jss there are loaded as ESM
  • Greatly improved build times by removing the gts clean step before compilation and using tsc -b
  • Renamed the bare relatively imports (e.g. ./path -> ./path.js) and fixed the directory imports (/ -> /index.js)
  • Refactored the system-tests to use pack-n-play
    • The integration tests uses a local server for optimal testing; no more flaky external server

🦕

@danielbankhead danielbankhead requested a review from a team as a code owner October 29, 2024 02:06
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Oct 29, 2024
@sofisl
Copy link
Contributor

sofisl commented Oct 29, 2024

I think my only concern with not using babel is that we have some unique plugins we wrote for additional functionality. I'm also concerned that if there's an error during compilation (or installation or whatever), the post-compile script won't run and then the library would fail. Seems safer to just convert those files to .cjs but I'm curious why you're interested in getting rid of babel.

@danielbankhead
Copy link
Contributor Author

@sofisl With this strategy we can avoid babel and its added complexity of maintaining it. Are there specific features we need here that are not present?

that if there's an error during compilation (or installation or whatever), the post-compile script won't run and then the library would fail

I'm not sure I understand - if the compilation fails then library is already broken

Seems safer to just convert those files to .cjs

Unfortunately there are some gotchas with this approach; notably with renaming imports referencing these files. npm natively supports nested package.jsons and node respects the nearest package.json's type field, as outlined here:

Between the time the original doc was written and today the space has improved and simplified a ton.

@danielbankhead danielbankhead added the next major: breaking change this is a change that we should wait to bundle into the next major version label Oct 29, 2024
Copy link

Warning: This pull request is touching the following templated files:

@sofisl
Copy link
Contributor

sofisl commented Oct 30, 2024

  • We have three plugins for our babel extension that replace __dirname, esmock, and we toggle an ESM flag variable. Seems like this library doesn't need it but if we do need to add these features before they're introduced in node, we'd need to rely on these plugins.

  • You're right that if there's a failure in installation we probably have another problem anyways. My larger point is that the library doesn't work without also adding the package.json, so I don't think that should be a separate step in the typescript configuration. A simple fix would just be to add it to the end of the script, tsc -b ./tsconfig.json ./tsconfig.cjs.json && node utils/enable-esm.mjs

  • FWIW, babel does take care of renaming the extensions wherever it imports.

I'm down to try it once we move the node step into compilation (vs. postcompile).

Copy link
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

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

Please combine the compile & postcompile step.

@danielbankhead danielbankhead merged commit 352d61b into main Oct 30, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next major: breaking change this is a change that we should wait to bundle into the next major version size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants