add support for esm named exports

[MylesBorins]

Through an ESM wrapper and package.exports this change allows the
semver module to be both imported and required while still supporting
legacy versions of Node.js. An important advantage to this change is that
semver will now support named imports, which are unsupported in commonjs
modules.

One thing to note. This change introduces the package.exports field
which locks down what modules can be deeply imported.

Currently support is maintained to allow deep imports of all files
currently included in package.files to keep the change Semver Minor.
In the future this interface could be further locked down.

[MylesBorins]

Oh ok, maybe I was just confused because it wasn't failing as expected on 10.x, glad to hear that is working as expected

…

On Thu, May 7, 2020, 12:32 PM Corey Farrell ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In esm-wrapper.mjs <#325 (comment)>: > +const Comparator = require('./classes/comparator'); +const Range = require('./classes/range'); +const satisfies = require('./functions/satisfies'); +const toComparators = require('./ranges/to-comparators'); +const maxSatisfying = require('./ranges/max-satisfying'); +const minSatisfying = require('./ranges/min-satisfying'); +const minVersion = require('./ranges/min-version'); +const validRange = require('./ranges/valid'); +const outside = require('./ranges/outside'); +const gtr = require('./ranges/gtr'); +const ltr = require('./ranges/ltr'); +const intersects = require('./ranges/intersects'); +const simplifyRange = require('./ranges/simplify'); +const subset = require('./ranges/subset'); + +export default { Not when I tested my suggestion locally on node.js 14.1.0, node_modules/semver was ignored by the self referential import and require. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#325 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYVZNXQNAI4PVW5DQS6DRQLPB7ANCNFSM4M22VZVA> .

[isaacs]

This LGTM, assuming that it still has full coverage after making the requested modification to the tests.

[isaacs]

Just added a test for map.js, which wasn't getting tested right now.

Rebase this onto master, and let the test guide you, and all will be good :)

[coveralls]

Coverage remained the same at 100.0% when pulling 8b3684d on MylesBorins:add-esm into e79ac3a on npm:master.

[MylesBorins]

Updated based on all the feedback. Currently have the single test per file but this is now going to fail on all version of node lower than 14 out of the box (for now). Before I was using a proxy cjs module to sniff the semver and skip the esm test on node.js older than 14. How would you suggest to unbreak older versions without compromises the 1 test per file system?

[coreyfarrell]

This LGTM, assuming that it still has full coverage after making the requested modification to the tests.

On it's own nyc will not produce coverage for ES modules loaded by node.js core. To get that you would need the experimental @istanbuljs/esm-loader-hook. I'm not sure getting coverage reported for the one statement in the esm wrapper is worth the trouble/risk of future breakage by using --experimental-loader.

[coreyfarrell]

One more comment on ESM coverage, the experimental module I mentioned does not work with nyc 14 (used by the current version of tap). It's @isaacs call but I think not having automated coverage for the esm wrapper should be fine, being a single statement means that we can see that it's fully covered.

As for the issue with ignoring the test in node.js < 13 I think you could:

• Add "test-ignore": "test/esm-wrapper.mjs" to package.json#tap
• Add to test/index.js:

if (semver.gte(process.version, '14.0.0')) {
  t.spawn(process.execPath, require.resolve('./esm-wrapper.mjs'))
}

Personally I would lean towards renaming the esm-wrapper.mjs files to index.mjs. AFAIK the map.js settings are applied per test run directly by tap and not to tests run by t.spawn. Doesn't make a difference now but eventually we'll be able to get coverage. Since test/index.js will filter coverage to index.js and index.mjs, that same filter would be inherited in the t.spawn of test/index.mjs.

[coreyfarrell]

I'm confused, why did you remove the --no-esm?

[MylesBorins]

the t.spawn shells out to node not tap so it doesn't end up using the built in runner (and esm). At least that is my understanding

[isaacs]

It'd be better to put "tap": { "esm": false } in the package.json than have it hard-coded in the command, imo.

[coreyfarrell]

Fair enough. I was just seeing the esm module as unnecessary but I guess it's also not strictly required to disable it in this repository.

[isaacs]

Yeah, missing coverage for a one-line file is not the end of the world, especially if we can be reasonably sure that it's working properly by some other means.

You can add the test file but include it in test-ignore like @coreyfarrell suggests, and then load it on versions that will do the right thing, that's fine. The nice/hazardous thing about a coverage-map is that if the test isn't run, it's as if its covered file just doesn't exist, so you still get 100%.

[MylesBorins]

Are folks happy with what we currently have?

…

On Fri, May 8, 2020, 1:46 PM isaacs ***@***.***> wrote: Yeah, missing coverage for a one-line file is not the end of the world, especially if we can be reasonably sure that it's working properly by some other means. You can add the test file but include it in test-ignore like @coreyfarrell <https://github.com/coreyfarrell> suggests, and then load it on versions that will do the right thing, that's fine. The nice/hazardous thing about a coverage-map is that if the test isn't run, it's as if its covered file just doesn't exist, so you still get 100%. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#325 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYV3E6XOTVF3VSUVS5STRQRAQ7ANCNFSM4M22VZVA> .

[mbruning24]

Currently working on an upgrade to angular 10 for our application, and node-semver is a dependency we have, it's throwing a lot of errors not having named exports, seems like the PR is basically good to go, can we get a bump?

[basz]

Not sure this is related, but I'm seeing an error in production builds only.

"Error: Cannot find module './ranges/outside'"

When I compare the node_modules/semver/ranges directories it is indeed missing an outside.js file.

Any way around this?

v7.3.2

[basz]

nvm. It was an incorrect ignore in ember-electron, filtering all files staring with 'out'...

[cawa-93]

Is there any updates? It seems to me that this could help the packages above to get around the problem of cyclic dependencies.

[MylesBorins]

Closing in lieu of #383