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: added support for ESM config files #982

Closed
wants to merge 1 commit into from
Closed

Conversation

corevo
Copy link
Contributor

@corevo corevo commented Nov 15, 2021

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

Added support for config files that are ESM, usually the migrations themselves are generated so they are commonjs.
But on the other hand the config could be in ESM in case the backend uses ESM.

This PR fixes that by trying to import the config file as an ECMAScript module first, and if it fails it fallbacks to CommonJS.

I've also had to change the babel config, so that it won't transpile the dynamic import to CommonJS as well.

Let me know if a change to the docs is necessary, and if so where to put it.

This PR also means that the new tests will fail on Node versions older than 12.20, but the failure will be graceful and CommonJS will work instead, since ESM doesn't work in Node versions earlier than that.

.babelrc Show resolved Hide resolved
@WikiRik
Copy link
Member

WikiRik commented Nov 16, 2021

Hi, thanks for the PR. You can update the docs by making a PR for this file in the main sequelize repo; https://github.com/sequelize/sequelize/blob/main/docs/manual/other-topics/migrations.md

Can you show how it fails on Node <12.20? We will be dropping support for Node 10 soonish, so I'm wondering if we should put this on hold until then or that we can merge this with the next release.

@corevo
Copy link
Contributor Author

corevo commented Nov 16, 2021

If you use Node version earlier than 12.20, you'll get the following error Error: SyntaxError: Unexpected token export, that's because these versions do not support native ESM, so they can't interpret the import and export keywords.

But if I run the other tests they pass

image

In essence .mjs is broken for all versions of Node supported (10, 12, 14, 16, 17), but I can only fix it for newer ones (12, 14, 16, 17). This does not introduce a new regression, rather fixes a bug in newer versions of Node.

For users still using Node 10 sequelize cli will work exactly the same way, since all the other tests pass, with the exception of the ESM tests, which won't work in Node 10 anyway.

@corevo
Copy link
Contributor Author

corevo commented Nov 16, 2021

I'll create a PR for the docs and mention this PR, do you want me to add code to skip these tests in case the runtime is Node 10?

@WikiRik
Copy link
Member

WikiRik commented Nov 16, 2021

I'll create a PR for the docs and mention this PR, do you want me to add code to skip these tests in case the runtime is Node 10?

You read my mind, I was about to ask that. Would be great if you could do that and then I'll assign a maintainer to look at this PR

@corevo
Copy link
Contributor Author

corevo commented Nov 16, 2021

@WikiRik I didn't know where to put describeOnlyForESM, I can move it to test/support/helpers.js if you want.

I've also opened sequelize/sequelize#13669 for the doc changes.

@sdepold
Copy link
Member

sdepold commented Nov 22, 2021

I think we can get to this from Wednesday on :) we'll discuss which node versions to support on Tuesday

@mariusa
Copy link

mariusa commented Dec 29, 2021

ping & thanks

@ephys
Copy link
Member

ephys commented Dec 29, 2021

The current solution is going to swallow errors if the file is esm but throws an error.

Instead of a fallback to require, I'd opt for feature-detecting import(): https://stackoverflow.com/questions/60317251/how-to-feature-detect-whether-a-browser-supports-dynamic-es6-module-loading

@corevo
Copy link
Contributor Author

corevo commented Dec 29, 2021

We can try to detect specific ESM errors and re-throw if the module itself errors.

@ephys
Copy link
Member

ephys commented Dec 29, 2021

import() throws Error: Not supported but detecting where it comes from would require looking at the stack trace.

We can detect whether import() is supported without using eval by requireing a file that exports import('fs').

If the require fails, or the exported promise rejected, import() is not supported.

@mariusa
Copy link

mariusa commented Feb 22, 2022

This wasn't merged, right?

On a new project using type=module, ran npx sequelize-cli init and it generated models/index.js with

module.exports = db;

@ephys
Copy link
Member

ephys commented Feb 22, 2022

It was (through #987) but it only impacts being able to load esm config files, generation of files hasn't been updated yet

@mariusa
Copy link

mariusa commented Feb 22, 2022

Ah, thanks for clarification

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

Successfully merging this pull request may close these issues.

5 participants