From b1ea44f08273091df95462a169a3310eee92313b Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Wed, 14 Jun 2023 16:36:58 -0700 Subject: [PATCH 1/2] Fix double-loading packages on npm --- CHANGELOG.md | 5 +++++ lib/src/npm.dart | 27 ++++++++++++++++++++++++++- pubspec.yaml | 2 +- test/npm_test.dart | 11 +++++++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 261d54d..5beca1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.4.7 +g +* Fix a bug where npm packages could crash on Node.js if loaded both through + `require()` and `import`. + ## 2.4.6 * Properly mark NPM packages as `"type": "module"` when `pkg.jsEsmExports` is diff --git a/lib/src/npm.dart b/lib/src/npm.dart index 6eebbb9..7d8acc4 100644 --- a/lib/src/npm.dart +++ b/lib/src/npm.dart @@ -579,7 +579,11 @@ void _writePlatformWrapper(String path, JSRequireSet requires, {bool node = false}) { var exports = jsEsmExports.value; if (exports != null) { - _writeImportWrapper('$path.${node ? 'mjs' : 'js'}', requires, exports); + if (node) { + _writeNodeImportWrapper('$path.mjs', exports); + } else { + _writeImportWrapper('$path.${node ? 'mjs' : 'js'}', requires, exports); + } _writeRequireWrapper('$path.${node ? 'js' : 'cjs'}', requires); } else { _writeRequireWrapper('$path.js', requires); @@ -613,6 +617,27 @@ String _loadRequires(JSRequireSet requires) { return buffer.toString(); } + +/// Writes a wrapper to [path] that loads and re-exports `$_npmName.node.js` +/// using ESM imports. +/// +/// Rather than having a totally separate ESM wrapper, for Node we load ESM +/// exports *through* the require wrapper. This ensures that we don't run into +/// issues like sass/dart-sass#2017 if both are loaded in the same Node process. +/// +/// [exports] is the value of [jsEsmExports]. +void _writeNodeImportWrapper( + String path, Set exports) { + var cjsUrl = './' + p.setExtension(p.basename(path), '.js'); + var buffer = StringBuffer("import cjs from ${json.encode(cjsUrl)};\n\n"); + + for (var export in exports) { + buffer.writeln("export const $export = cjs.$export;"); + } + + writeString(path, buffer.toString()); +} + /// Writes a wrapper to [path] that loads and re-exports `$_npmName.dart.js` /// using ESM imports with [requires] injected. /// diff --git a/pubspec.yaml b/pubspec.yaml index 8e83253..47b59e1 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: cli_pkg -version: 2.4.6 +version: 2.4.7 description: Grinder tasks for releasing Dart CLI packages. homepage: https://github.com/google/dart_cli_pkg diff --git a/test/npm_test.dart b/test/npm_test.dart index 05310e0..25bf28d 100644 --- a/test/npm_test.dart +++ b/test/npm_test.dart @@ -701,6 +701,11 @@ void main() { const myApp = require("my_app"); console.log(myApp.hello); + """), + // Regression test for sass/dart-sass#2017 + d.file("both.mjs", """ + import "./test.mjs"; + import "./test.cjs"; """) ]).create(); @@ -717,6 +722,12 @@ void main() { await TestProcess.start("node$dotExe", [d.path("depender/test.cjs")]); expect(cjsProcess.stdout, emits("true")); await cjsProcess.shouldExit(0); + + var bothProcess = + await TestProcess.start("node$dotExe", [d.path("depender/both.mjs")]); + expect(bothProcess.stdout, emits("true")); + expect(bothProcess.stdout, emits("true")); + await bothProcess.shouldExit(0); }); test("overwrite existing string value", () async { From 1b0ee1a9458f5eac8347d223a00999e53ddc521c Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Wed, 14 Jun 2023 16:38:10 -0700 Subject: [PATCH 2/2] Reformat --- lib/src/npm.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/src/npm.dart b/lib/src/npm.dart index 7d8acc4..679c1fc 100644 --- a/lib/src/npm.dart +++ b/lib/src/npm.dart @@ -617,7 +617,6 @@ String _loadRequires(JSRequireSet requires) { return buffer.toString(); } - /// Writes a wrapper to [path] that loads and re-exports `$_npmName.node.js` /// using ESM imports. /// @@ -626,8 +625,7 @@ String _loadRequires(JSRequireSet requires) { /// issues like sass/dart-sass#2017 if both are loaded in the same Node process. /// /// [exports] is the value of [jsEsmExports]. -void _writeNodeImportWrapper( - String path, Set exports) { +void _writeNodeImportWrapper(String path, Set exports) { var cjsUrl = './' + p.setExtension(p.basename(path), '.js'); var buffer = StringBuffer("import cjs from ${json.encode(cjsUrl)};\n\n");