Repository navigation
commonjs: v19.0.0 breaks semver package #879
Description
- 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
Reacted by Saran Tanpituckpong ✔️, Alex Kozack, Erez Rokah, Hans Otto Wirtz and Francois Wouts
Reacted by Lukas Taegert-Atkinson
Activity
lukastaegert commentedon May 10, 2021
Ok, so here is what is going on:
The problem is of course rooted in the cyclic dependency between
rangeandcomparator. 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,
Comparatoris defined beforeRange, but after bundling the order is switched. This is due to the "hack" of moving allrequireexpressions to the end incomparatorandrange. 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.exportsfor the exports of those files (or for files in cyclic dependencies in general). Instead, export e.g.Rangeas(module.)exports.Rangeand the same forComparator. And access them as properties of the imported object. E.g.This will not fix the execution order but since you are not overwriting
module.exportsitself, it will use the hoistedexportsobject 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 commentedon Jul 10, 2021
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. ⓘ
The semver package as problems with cyclical dependencies internally
The semver package as problems with cyclical dependencies internally