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

V7 #241

Merged
merged 10 commits into from
Aug 1, 2020
Merged

V7 #241

merged 10 commits into from
Aug 1, 2020

Conversation

davidtheclark
Copy link
Collaborator

@davidtheclark davidtheclark commented Jul 19, 2020

Changelog entry:

  • Breaking change: Add ${moduleName}rc.cjs and ${moduleName}.config.cjs to the default searchPlaces, to support users of "type": "module" in recent versions of Node.
  • Breaking change: Drop support for Node 8. Now requires Node 10+.

After looking a little bit into .mjs support, I'm not inclined to add it to the default searchPlaces & loaders, for a few reasons:

  • Unlike all the other default loaders, it cannot be done synchronously without a hack (right?).
  • That means the default searchPlaces would vary between cosmiconfig and cosmiconfigSync, which would be confusing and annoying.
  • .mjs not only wouldn't work with cosmiconfigSync: it also wouldn't work with some versions of Node that Cosmiconfig supports.
  • That means we'd have to write more codepaths and validation to ensure it's going to work as hoped, I think.
  • None of this seems worthwhile for the sake of import and export keywords, especially since support is still considered Stability 1: Experimental even in Node 14.

I'll let this PR sit for a couple of days to see if anybody has any feedback, or wants to show a good way to add .mjs support.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2020

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #241   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          227       227           
  Branches        46        46           
=========================================
  Hits           227       227           
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b30bde...e463b0f. Read the comment docs.

@vvanpo
Copy link

vvanpo commented Jul 21, 2020

None of this seems worthwhile for the sake of import and export keywords

I think a lot of people are going to disagree with this. Developers want consistency, and that means using one module system within a project. ES2015 has gained massive adoption, and modules have been widely supported in browsers for a number of years. Node.js may have lagged behind a bit, but they've removed the flag and enabled it by default, regardless of its official stability designation.

It's pretty clear that the future for Javascript modules is ESM, not CommonJS. Many projects write all the code they can as ES modules, only to have a few config files that are simple wrappers around require('@babel/register') and re-exporting some other file that contains the actual configuration. "type": "module" is pretty compelling to a lot of people for this reason, and config files are really the only snag that stops projects from being able to adopt it. Because cosmiconfig is such a popular dependency for configuration management, this project will have a significant influence over whether "type": "module" sees swift and seamless adoption, or not.

So given a future where ESM is a guarantee, and where CommonJS usage will gradually disappear, we're going to have to pull the plug on the synchronous API eventually. I think it's prudent to start discouraging its use now, to lessen the impact of deprecating it in the future.

@davidtheclark
Copy link
Collaborator Author

davidtheclark commented Jul 21, 2020

@vvanpo Unfortunately, I suspect that plenty of users would not be happy about pulling the plug on the synchronous API. (I'd love to hear that this opinion has changed.) That seems to me the priority feature for a couple of reasons: (a) it already exists; (b) it is not possible (without nasty hacks) to extend the asynchronous API and make it synchronous, while it is very, very possible to implement an .mjs loader — even publish a wrapper around cosmiconfig that implements an .mjs loader by default.

That said, I realize that in my eagerness to use the native import() I'd forgotten that https://github.com/standard-things/esm exists. I wonder if that will solve this problem. I will try it out — or PR more than welcome!

@vvanpo
Copy link

vvanpo commented Jul 22, 2020

@davidtheclark Unfortunately that project doesn't appear to be maintained (last commit was almost a year ago). I tried playing around with it, and immediately ran into a major bug. Even if it was working, using it would enable some behaviours that could never be replicated using native ESM in Node.js (synchronous imports of ES modules, as well as detecting CJS vs. ESM based on file contents, not the package's type field).

Unfortunately, I suspect that plenty of users would not be happy about pulling the plug on the synchronous API.

Of course, and I would never suggest dropping support for it while it still has lots of users. But if we allow the two APIs to diverge slightly by supporting ESM in the asynchronous API, I believe usage of the synchronous API will eventually drop off enough that it can be deprecated.

Even if supporting .mjs sounds like it will require too many caveats and extra work, supporting just import() of .js configs solely for the case of "type": "module" should not be a breaking change (as currently it is impossible to use a .js file with cosmiconfig at all if your project defines "type": "module") and would make a lot of users happy.

Although I've never worked on cosmiconfig before, I'd be happy to attempt a pull request for making "type": "module" work with .js imports in the asynchronous API.

},
"engines": {
"node": ">=8"
"node": ">=10"
Copy link

Choose a reason for hiding this comment

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

Since support for node@8 was dropped, should the @babel/preset-env targets be updated to { node: '10' } as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good find.

@davidtheclark
Copy link
Collaborator Author

Thanks for the warnings about esm.

I'm definitely open to a PR by someone invested in the feature 👍

@Bnaya
Copy link

Bnaya commented Jul 31, 2020

Thanks!
Is this released somewhere? (next dist tag or so?)

@davidtheclark
Copy link
Collaborator Author

I will release v7 today, instead of holding back any longer for .mjs, since this release unblocks anybody using type: "module".

@davidtheclark davidtheclark merged commit 71289f8 into master Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants