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: non-breaking ESM support 🤝 #613

Merged
merged 38 commits into from
Dec 8, 2023
Merged

feat: non-breaking ESM support 🤝 #613

merged 38 commits into from
Dec 8, 2023

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Aug 2, 2023

Closes #608

This adds some esm-vs-commonjs "smartness" to umzug:

  1. use require for .cjs migrations and import for .mjs migrations (and their typescript equivalents)
  2. use require for .js migrations if typeof require.main === 'object', and import to .js migrations otherwise
  3. use the same criteria to create (c)js vs mjs templates when creating migration files
  4. add "moduleResolution": "Node16" to tsconfig.lib.json to make sure import(filepath) doesn't get transpiled into __importStar(require(filepath)) (see here and here)

Tests/examples:

  • add a vanilla-esm example to make sure using import / top-level await works
  • add a step to the test_pkg job to make sure vitest isn't hiding gnarly import problems - this is installing the compiled library as a .tgz, and with no other dev/prod dependencies like vitest or ts-node having been installed, so should be very close to what end users will do

Didn't:

  • add a wrapper.mjs file in the compiled folder as suggested in Migrate to ESM-first #608 (comment), mostly just because it didn't seem to be necessary? It seems to work fine when imported from an ES-module, using top-level await, etc., even though umzug is itself a commonjs module.
original body

Related to #608 - although does not close it.

This adds built-in support for .mjs and .mts files. .mjs should "just work" - write migrations as ESM modules and they'll be imported in the right way. For the current major version, .js will continue to be assumed commonjs. ESM-fans will just have to type that extra m in their filenames.

This PR doesn't add a wrapper file so that the umzug library itself can be imported as an ES module. That can be done in a follow-up PR. In the meantime, ESM users can use createRequire as in the existing ESM example.

@mmkal mmkal marked this pull request as draft August 3, 2023 07:34
@mmkal

This comment was marked as outdated.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #613 (8d2e6fa) into main (d5cf0c3) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 8d2e6fa differs from pull request most recent head 1dfa1d1. Consider uploading reports for the commit 1dfa1d1 to get more accurate results

@@           Coverage Diff           @@
##             main     #613   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files          12       12           
  Lines        1409     1424   +15     
  Branches      255      261    +6     
=======================================
+ Hits         1403     1418   +15     
  Misses          6        6           
Files Coverage Δ
src/umzug.ts 99.22% <100.00%> (+0.02%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@mmkal mmkal changed the title feat: support .mjs and .mts migrations feat: non-breaking ESM support Nov 6, 2023
@mmkal mmkal changed the title feat: non-breaking ESM support feat: non-breaking ESM support 🤝 Nov 6, 2023
@mmkal mmkal marked this pull request as ready for review November 6, 2023 18:42
Copy link

github-actions bot commented Nov 6, 2023

Released in v3.5.0-0.

@mmkal
Copy link
Contributor Author

mmkal commented Nov 6, 2023

@ephys @SmashingQuasar @JSteunou @mysuf @papb since you've all commented on #608 and/or #626. I've published this PR, which I hope adds what we can consider "first-class" ESM support, as 3.5.0-0 (under the beta tag). Having tested it, it seems to work as expected even without a wrapper .mjs file in the lib/ directory.

If you're able, would you be able to try this version and see if it works for you? It does not migrate to ESM-first, but it now uses import(...) instead of require(...) when it detects ESM migrations. I also added some tests to make sure import { Umzug } from 'umzug' and top-level await work, etc.

Worth reading in the PR description, but this doesn't change umzug to be a "type": "module" package itself. IMO, there's no need for that, even in 2023. As long as ESM users can use it without having to write custom resolvers etc.

@mmkal
Copy link
Contributor Author

mmkal commented Nov 6, 2023

Also tagging @WikiRik @sdepold as an FYI

@SmashingQuasar
Copy link

@ephys @SmashingQuasar @JSteunou @mysuf @papb since you've all commented on #608 and/or #626. I've published this PR, which I hope adds what we can consider "first-class" ESM support, as 3.5.0-0 (under the beta tag). Having tested it, it seems to work as expected even without a wrapper .mjs file in the lib/ directory.

If you're able, would you be able to try this version and see if it works for you? It does not migrate to ESM-first, but it now uses import(...) instead of require(...) when it detects ESM migrations. I also added some tests to make sure import { Umzug } from 'umzug' and top-level await work, etc.

Worth reading in the PR description, but this doesn't change umzug to be a "type": "module" package itself. IMO, there's no need for that, even in 2023. As long as ESM users can use it without having to write custom resolvers etc.

You efforts are very much appreciated. Thanks a lot for taking time to look into this. It will benefit everyone in the long run!
I'll have a look whenever possible, we are a bit behind right now so it may not be an immediate thing.

@mysuf
Copy link

mysuf commented Nov 9, 2023

@mmkal Thank you. I've tried and in my case it works out of the box without custom resolver as expected.

@mmkal mmkal mentioned this pull request Nov 15, 2023
@zevisert
Copy link

3.5.0-0 worked in my project that's working on an ESM migration with no other changes as well!

@mmkal
Copy link
Contributor Author

mmkal commented Dec 8, 2023

Feedback looks good, I'm merging this. 🤞

@mmkal mmkal merged commit 86acdd6 into main Dec 8, 2023
10 checks passed
@mmkal mmkal deleted the mjs branch December 8, 2023 01:07
@JSteunou
Copy link

JSteunou commented Dec 8, 2023

@mmkal great job, really appreciate the effort! 👏

Copy link

Released in v3.5.0.

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

Successfully merging this pull request may close these issues.

Migrate to ESM-first
5 participants