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: add support for ESM config files #987

Merged
merged 4 commits into from
Jan 11, 2022
Merged

feat: add support for ESM config files #987

merged 4 commits into from
Jan 11, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Jan 8, 2022

Description of change

Continuation of PR #982

This PR uses feature-detection to detect whether we can use import() to import the configuration file.
It also fixes the babel config to properly transpile import-helper.js for node 10.

Documentation PR is still sequelize/sequelize#13669

Unit tests fail but I think it's unrelated to this PR. Need to investigate.

Also pinging @corevo :)

@ephys ephys requested a review from WikiRik January 8, 2022 10:41
@ephys ephys self-assigned this Jan 8, 2022
@WikiRik
Copy link
Member

WikiRik commented Jan 8, 2022

Failing unit test is indeed not your fault, but is something we should look into

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Seems to look good. Will wait a few days for @corevo to respond to this as well. Can you change the title of the PR so we can squash this when merging?

@ephys ephys changed the title Added support for ESM config files (cont.) feat: add support for ESM config files Jan 8, 2022

// mimics what `import()` would return for
// cjs modules.
return { default: require(modulePath) };
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that the module only has a default export, and is not compatible with named exports.

For the purposes of the config that's fine, because the config is exported on the default export when is ESM compatible, but this is not true for all cases.

Which makes me feel like it would be better to invoke supportsDynamicImport in config-helper.js and either use the import helper, or straight up require.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean.

If the user is using named exports then the platform is compatible with ESM and import() will be used, which supports named exports.

If they're using CJS, there are no named-exports.

Native import() of a CJS file already returns { default: module.exports }.

Copy link
Contributor

@corevo corevo Jan 11, 2022

Choose a reason for hiding this comment

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

I understand what you mean, I think I didn't fully understand what you were saying in #982

But basically you wanted to write a node 10 compatible importModule function, while I was under the impression that you were going to drop support for node 10.

EDIT: by you, I mean the sequelize team

Copy link
Member

Choose a reason for hiding this comment

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

We'll indeed drop Node 10 for v7, but I don't think we want to wait with fixing this until the core sequelize package is ready for that

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, makes sense, I think we can resolve this then.

Copy link
Member

Choose a reason for hiding this comment

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

Great, so that's an approval from you? Then I think we can merge and set up a new release

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly :) The point was to replicate import() using require, the code that uses import-helper is then responsible for properly using the module

Copy link

Choose a reason for hiding this comment

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

The point was to replicate import() using require

For sequelize v7, will this solution be dropped and using the native import be supported instead? Along with .sequelizerc no longer having module.exports ? Should I open an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the next major will drop support for node 10 so import() will always be used.

Supporting ESM in .sequelizerc could already be done in this version though. It should be a small change too.
Simply need to make this line use the new import helper

? JSON.parse(JSON.stringify(require(rcFileResolved)))
+ support searching for .sequelizerc + .sequelizerc.json + .sequelizerc.cjs + .sequelizerc.mjs + .sequelizerc.js.

(feel free to open an issue)

Copy link

Choose a reason for hiding this comment

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

Thanks! I've openeded #989 , which is only for v7.
I think current version is fine as is, would prefer focusing on v7 since sequelize v7 is almost ready.

@WikiRik
Copy link
Member

WikiRik commented Jan 11, 2022

@ephys I'll leave the merging up to you

@ephys ephys merged commit 54b9ded into main Jan 11, 2022
@ephys ephys deleted the feature/esm-config branch January 11, 2022 17:52
@ephys
Copy link
Member Author

ephys commented Jan 11, 2022

Here we go 🎉

@github-actions
Copy link

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mariusa
Copy link

mariusa commented Jan 18, 2022

Can https://sequelize.org/master/manual/migrations.html#dynamic-configuration be used with ESM now that #987 has been merged?

According to docs, this means

  1. config.js ESM support (there's no example for config.js with ESM syntax)
  2. .sequelize.rc telling sequelize to use config.js instead of config.json (no support yet? Or just not documented?)

@ephys
Copy link
Member Author

ephys commented Jan 19, 2022

The documentation PR hasn't been merged yet but it should work (sequelize/sequelize#13669)

@mariusa
Copy link

mariusa commented Jan 19, 2022

Thanks. That PR doesn't have ESM examples for config.js / .sequelize.rc. I don't know how to convert existing format to ESM.

@ephys
Copy link
Member Author

ephys commented Jan 19, 2022

.sequelizerc itself can't be ESM yet (#990)

config.js can be ESM the same way other ESM files are handled in Node:

  • either use the .js file extension and set "type": "module" in package.json
  • or use the .mjs file extension

and the configuration should be the default export.

I'll update the PR with this comment. Is there any other information you need to add to the doc?

@vishnu-buildd
Copy link

ESM with sequelize has been a little confusing for me. I tried going through the issues in sequelize and sequelize-cli and my understanding is that we can write ESM code with sequelize but not for migrations using the CLI. The sequelizerc file is the limitation? Can we have consolidated information in one page of the documentation. More than happy to help out too.

@ephys
Copy link
Member Author

ephys commented Mar 19, 2022

I agree that it's confusing. It's mainly due to us not having enough bandwidth available right now to improve this project as we're hard at work on the main repository

What I'd like to do is rewrite this project as esm and add native support for esm everywhere, as a major release that drops node < 12.

@vishnu-buildd
Copy link

vishnu-buildd commented Mar 19, 2022

Sounds good to me. I think every node project should ideally support/adopt ESM as standard. It's the way forward. I mean I rewriting an sdk that uses sequelize at work to ESM modules. That's how I ended up here.

NB: sorry to ping all the people in this closed PR.

@Falinor
Copy link

Falinor commented Jun 12, 2022

It seems that this PR broke the usage of TypeScript with sequelize-cli 6.4.0.
I think it may be related to the support for ESM which breaks the reading of config.ts.

$ <project>/node_modules/.bin/sequelize db:migrate

Sequelize CLI [Node: 16.14.0, CLI: 6.4.0, ORM: 6.20.1]

ERROR: Error reading "sequelize/config.ts". Error: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for <project>/sequelize/config.ts

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.

6 participants