node-semver icon indicating copy to clipboard operation
node-semver copied to clipboard

add support for node.js esm auto exports

Open MylesBorins opened this issue 3 years ago • 3 comments

This is an alternative to https://github.com/npm/node-semver/pull/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.

MylesBorins avatar May 13 '21 14:05 MylesBorins

Coverage Status

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

coveralls avatar May 13 '21 14:05 coveralls

Any thoughts on merging this in?

btakita avatar Jul 21 '21 02:07 btakita

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

ljharb avatar Jul 21 '21 03:07 ljharb

Is there anything preventing this PR from being merged?

rxliuli avatar Oct 01 '22 15:10 rxliuli

@rxliuli merging this PR would mean that multiple packages in the ecosystem would need to follow it, and old versions never would - whereas https://github.com/npm/node-semver/pull/383#issuecomment-883865501 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.

ljharb avatar Oct 01 '22 16:10 ljharb

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

MylesBorins avatar Oct 03 '22 18:10 MylesBorins

@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.

guybedford avatar Oct 03 '22 18:10 guybedford

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

ljharb avatar Oct 03 '22 19:10 ljharb

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.

guybedford avatar Oct 03 '22 19:10 guybedford

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.

ljharb avatar Oct 03 '22 19:10 ljharb

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.

guybedford avatar Oct 03 '22 19:10 guybedford

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.

wraithgar avatar Oct 04 '22 18:10 wraithgar