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

fix: add support for node.js esm auto exports #383

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

MylesBorins
Copy link
Contributor

This is an alternative to #325

Node.js now supports ESM named exports via auto-detection, but the way node-semver was creating module.exports was breaking that algorithm. The advantage of this approach is that there is no need for a wrapper file or for package.exports. It should be completely Semver Minor

This is a minor tweak to the layout of index.js that allows for ESM named exports support in all modern Node.js ESM implementations.

We could borrow the tests from #325, but it it a bit obtuse to add a single test to the testing framework as there is a 1:1 file to test expectation.

@coveralls
Copy link

coveralls commented May 13, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling b035c1d on MylesBorins:alternative-esm into e79ac3a on npm:master.

@btakita
Copy link

btakita commented Jul 21, 2021

Any thoughts on merging this in?

@ljharb
Copy link

ljharb commented Jul 21, 2021

I wonder if perhaps cjs-module-lexer could instead be updated to understand this pattern?

@farahmandakbar farahmandakbar linked an issue May 12, 2022 that may be closed by this pull request
@rxliuli
Copy link

rxliuli commented Oct 1, 2022

Is there anything preventing this PR from being merged?

@ljharb
Copy link

ljharb commented Oct 1, 2022

@rxliuli merging this PR would mean that multiple packages in the ecosystem would need to follow it, and old versions never would - whereas #383 (comment) would mean that every version of any package following this package's pattern would Just Work - i don't think one-off changes to packages are a sustainable approach.

@MylesBorins
Copy link
Contributor Author

I still think we should land this @darcyclarke thoughts?

@ljharb I get where you are coming from regarding a philosophical approach here... but this change will in fact fix a usability issue with the package and is ready to land, not landing it on principal seems like a mistake. That doesn't mean we should try and fix this in the lexer as well. /cc @guybedford

@guybedford
Copy link

@MylesBorins because of backporting constraints, I don't think lexer features should be changed. Object detection is just patchy unfortunately. I think we should only stick to updates for parser bug fixes that break new language syntax or fail on valid language syntax.

@ljharb
Copy link

ljharb commented Oct 3, 2022

@guybedford what would be the backport constraint here? wouldn't this just add new named exports when backported?

@guybedford
Copy link

The problem is we cannot expect users to track fine-grained exports detection rules as being compatible or not. For example if we made the lexer support these named exports in Node.js 18 only, it would be highly suprising when it breaks on older versions of Node.js, as opposed to other types of new features which can be clearly communicated as experimental.

@ljharb
Copy link

ljharb commented Oct 3, 2022

Understood - although that seems true of virtually any feature, whether labelled experimental or not, and the solution to all of them is to run CI on supported node versions.

@guybedford
Copy link

Yes, although the difference here is parser internals are difficult to communicate as features, in the same way that versioning parsers or transformers is hard.

index.js Outdated Show resolved Hide resolved
@wraithgar wraithgar changed the title add support for node.js esm auto exports fix: add support for node.js esm auto exports Oct 4, 2022
@wraithgar
Copy link
Member

Fixing this here fixes auto esm exports for the most highly downloaded package on the entire npm registry. Fixing the lexer is outside the scope of what node-semver needs to worry about today, and folks can go discuss that in the appropriate repo.

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.

require('semver')
7 participants