node-semver

node-semver copied to clipboard

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.

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 [email protected] wrote:

@coreyfarrell commented on this pull request.

In esm-wrapper.mjs https://github.com/npm/node-semver/pull/325#discussion_r421638004:

+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 https://github.com/npm/node-semver/pull/325#discussion_r421638004, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADZYVZNXQNAI4PVW5DQS6DRQLPB7ANCNFSM4M22VZVA .

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 :)

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

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?

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.

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.

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

Are folks happy with what we currently have?

On Fri, May 8, 2020, 1:46 PM isaacs [email protected] 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 https://github.com/npm/node-semver/pull/325#issuecomment-625935822, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADZYV3E6XOTVF3VSUVS5STRQRAQ7ANCNFSM4M22VZVA .

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?

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

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

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

Closing in lieu of https://github.com/npm/node-semver/pull/383