commonjs: v19.0.0 breaks semver package

[thorn0]

• Rollup Plugin Name: commonjs
• Rollup Plugin Version: 19.0.0
• Rollup Version: 2.47.0
• Operating System (or Browser): Ubuntu 20
• Node Version: v16.0.0
• Link to reproduction: https://github.com/thorn0/rollup-commonjs-semver-issue

Expected Behavior

"use strict";
const semver = require("semver");
console.log(semver.satisfies("4.2.4", ">= 4.0.0"));

This code prints true if executed with and without bundling.

Actual Behavior

After bundling it prints false. semver.satisfies always returns false now.

Additional Information

It worked as expected with the previous version of the plugin.

Related: #658, https://github.com/npm/node-semver/blob/master/classes/range.js

[lukastaegert]

Ok, so here is what is going on:
The problem is of course rooted in the cyclic dependency between range and comparator. Funnily enough, version 19 promises to improve cyclic dependency handling, so why did it get worse for you?

The problem is that even with Rollup 18, the execution order of the bundle is wrong for the those two files. When you run without bundling, Comparator is defined before Range, but after bundling the order is switched. This is due to the "hack" of moving all require expressions to the end in comparator and range. At the moment, Rollup cannot deal with this other than hoisting the imports to the top, which breaks the build. Due to many try-catch statements in your package, these kind of errors are all swallowed so it is really hard to debug, but that is what is happening.

This only manifests as an error in version 19 because this version now "snapshots" require expressions instead of treating them as hoisted variables. Though snapshotting is technically more correct in many cases, it no longer "fixes" the broken execution order for you. Unfortunately, it is not really easy (and questionable) to "reintroduce" this.

There is a really simple fix for you: Do not use assignments to module.exports for the exports of those files (or for files in cyclic dependencies in general). Instead, export e.g. Range as (module.)exports.Range and the same for Comparator. And access them as properties of the imported object. E.g.

const range = require('./range'); 
//... now access it as range.Range

This will not fix the execution order but since you are not overwriting module.exports itself, it will use the hoisted exports object that the commonjs plugin creates for your file which is always hoisted to the top. It will likely also make it unnecessary to move the require expressions past the class definitions.

I hope that Rollup will eventually also be able to handle the correct execution order in your case, but it will be complicated and costly as basically all files will be needed to be wrapped in additional function wrappers that are conditionally executed in the correct order.

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ