From 66b7dba5c138ad9377401ccf900ac75274bf3ee7 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 8 Jun 2023 14:45:28 -0700 Subject: [PATCH] Generate `"type": "module"` npm packages when ESM is enabled See sass/dart-sass#1995 --- CHANGELOG.md | 5 ++++ lib/src/npm.dart | 70 +++++++++++++++++++++++++++++----------------- pubspec.yaml | 2 +- test/npm_test.dart | 46 ++++++++++++++++++++++++++---- 4 files changed, 90 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 345ff2e..261d54d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.4.6 + +* Properly mark NPM packages as `"type": "module"` when `pkg.jsEsmExports` is + set, and mark all CJS files as explicitly `.cjs`. + ## 2.4.5 * Properly set the pub credentials for the latest versions of the Dart SDK. diff --git a/lib/src/npm.dart b/lib/src/npm.dart index 452b973..0f701d4 100644 --- a/lib/src/npm.dart +++ b/lib/src/npm.dart @@ -179,6 +179,22 @@ final npmDistTag = InternalConfigVariable.fn(() { return firstComponent is String ? firstComponent : "pre"; }); +/// Whether we're generating a package that supports ESM imports. +bool get _supportsEsm => jsEsmExports.value != null; + +/// The file extension for CommonJS files in the generated NPM package. +/// +/// If the NPM package supports ESM, we treat that as canonical and add an +/// explicit extension for CJS files. Otherwise, we treat CJS as canonical. +String get _cjs => _supportsEsm ? '.cjs' : '.js'; + +/// The file extension for ESM files in the generated NPM package. +/// +/// This is always `.js`, because we treat ESM as canonical if it's being +/// generated at all. We still store it as a variable to document that the files +/// are expected to be ESM. +const _mjs = '.js'; + /// Whether [addNpmTasks] has been called yet. var _addedNpmTasks = false; @@ -206,7 +222,7 @@ void addNpmTasks() { if (hasNonCliRequires) { fail("If jsModuleMainLibrary isn't set, all jsRequires must have " "JSRequireTarget.cli or JSRequireTarget.all."); - } else if (jsEsmExports.value != null) { + } else if (_supportsEsm) { fail("If jsEsmExports is set, jsModuleMainLibrary must be set as well."); } } @@ -350,7 +366,7 @@ Future _buildPackage() async { dir.createSync(recursive: true); var extractedRequires = _copyJSAndInjectDependencies( - 'build/$_npmName.dart.js', p.join(dir.path, '$_npmName.dart.js')); + 'build/$_npmName.dart.js', p.join(dir.path, '$_npmName.dart$_cjs')); var allRequires = _requiresForTarget(JSRequireTarget.all).union(extractedRequires); @@ -364,14 +380,14 @@ Future _buildPackage() async { jsonEncode({ ...npmPackageJson.value, "version": version.toString(), - "bin": {for (var name in executables.value.keys) name: "$name.js"}, + if (_supportsEsm) "type": "module", + "bin": {for (var name in executables.value.keys) name: "$name$_cjs"}, if (jsModuleMainLibrary.value != null) - "main": "$_npmName.${nodeRequires.isEmpty ? 'default' : 'node'}" - ".${jsEsmExports.value == null ? 'js' : 'cjs'}", + "main": "$_npmName.${nodeRequires.isEmpty ? 'default' : 'node'}$_cjs", if (npmPackageJson.value["exports"] is Map || nodeRequires.isNotEmpty || browserRequires.isNotEmpty || - jsEsmExports.value != null) + _supportsEsm) "exports": { if (npmPackageJson.value["exports"] is Map) ...npmPackageJson.value["exports"] as Map, @@ -389,33 +405,33 @@ Future _buildPackage() async { """); - if (jsEsmExports.value != null) { + if (_supportsEsm) { buffer.writeln(""" -require('./$_npmName.dart.js'); +require('./$_npmName.dart$_cjs'); var library = globalThis._cliPkgExports.pop(); if (globalThis._cliPkgExports.length === 0) delete globalThis._cliPkgExports; """); } else { - buffer.writeln("var library = require('./$_npmName.dart.js');"); + buffer.writeln("var library = require('./$_npmName.dart$_cjs');"); } buffer.writeln(_loadRequires(cliRequires.union(allRequires))); buffer.writeln( "library.${_executableIdentifiers[name]}(process.argv.slice(2));"); - writeString(p.join('build', 'npm', '$name.js'), buffer.toString()); + writeString(p.join('build', 'npm', '$name$_cjs'), buffer.toString()); } if (jsModuleMainLibrary.value != null) { if (nodeRequires.isNotEmpty) { - _writePlatformWrapper(p.join('build', 'npm', '$_npmName.node.js'), + _writePlatformWrapper(p.join('build', 'npm', '$_npmName.node'), nodeRequires.union(allRequires)); } if (browserRequires.isNotEmpty) { - _writePlatformWrapper(p.join('build', 'npm', '$_npmName.browser.js'), + _writePlatformWrapper(p.join('build', 'npm', '$_npmName.browser'), browserRequires.union(allRequires)); } - _writePlatformWrapper(p.join('build', 'npm', '$_npmName.default.js'), + _writePlatformWrapper(p.join('build', 'npm', '$_npmName.default'), defaultRequires.union(allRequires)); } @@ -467,7 +483,7 @@ JSRequireSet _copyJSAndInjectDependencies(String source, String destination) { var buffer = StringBuffer(); var exportsVariable = "exports"; - if (jsEsmExports.value != null) { + if (_supportsEsm) { buffer.writeln(""" // Because of vitejs/vite#12340, there's no way to reliably detect whether we're // running as a (possibly bundled/polyfilled) ESM module or as a CommonJS @@ -522,21 +538,23 @@ JSRequireSet _requiresForTarget(JSRequireTarget target) => /// Returns a single string specifier for `package.exports` if [jsEsmExports] /// isn't set, or a conditional export if it is. -Object _exportSpecifier(String name) => jsEsmExports.value == null - ? "./$_npmName.$name.js" - : {"require": "./$_npmName.$name.cjs", "default": "./$_npmName.$name.js"}; +Object _exportSpecifier(String name) => _supportsEsm + ? {"require": "./$_npmName.$name$_cjs", "default": "./$_npmName.$name$_mjs"} + : "./$_npmName.$name$_cjs"; -/// Writes one or two wrappers that loads and re-exports `$_npmName.dart.js` +/// Writes one or two wrappers that loads and re-exports `$_npmName.dart.[c]js` /// with [requires] injected. /// +/// The [requires] should not have the final `.[cm]js` extension. +/// /// This writes both an ESM and a CJS wrapper if [jsEsmExports] is set. void _writePlatformWrapper(String path, JSRequireSet requires) { var exports = jsEsmExports.value; if (exports != null) { - _writeImportWrapper(path, requires, exports); - _writeRequireWrapper(p.setExtension(path, '.cjs'), requires); + _writeImportWrapper('$path$_mjs', requires, exports); + _writeRequireWrapper('$path$_cjs', requires); } else { - _writeRequireWrapper(path, requires); + _writeRequireWrapper('$path$_cjs', requires); } } @@ -545,12 +563,12 @@ void _writePlatformWrapper(String path, JSRequireSet requires) { void _writeRequireWrapper(String path, JSRequireSet requires) { writeString( path, - (jsEsmExports.value == null - ? "const library = require('./$_npmName.dart.js');\n" - : "require('./$_npmName.dart.js');\n" + (_supportsEsm + ? "require('./$_npmName.dart$_cjs');\n" "const library = globalThis._cliPkgExports.pop();\n" "if (globalThis._cliPkgExports.length === 0) delete " - "globalThis._cliPkgExports;\n") + + "globalThis._cliPkgExports;\n" + : "const library = require('./$_npmName.dart$_cjs');\n") + "${_loadRequires(requires)}\n" "module.exports = library;\n"); } @@ -581,7 +599,7 @@ void _writeImportWrapper( buffer ..write(""" -import ${json.encode('./$_npmName.dart.js')}; +import ${json.encode('./$_npmName.dart$_cjs')}; const _cliPkgLibrary = globalThis._cliPkgExports.pop(); if (globalThis._cliPkgExports.length === 0) delete globalThis._cliPkgExports; diff --git a/pubspec.yaml b/pubspec.yaml index e139621..8e83253 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: cli_pkg -version: 2.4.5 +version: 2.4.6 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 eb9e7b9..4da30df 100644 --- a/test/npm_test.dart +++ b/test/npm_test.dart @@ -468,7 +468,7 @@ void main() { await (await grind(["pkg-npm-dev"])).shouldExit(); var process = await TestProcess.start( - "node$dotExe", [d.path("my_app/build/npm/exec.js")]); + "node$dotExe", [d.path("my_app/build/npm/exec.cjs")]); expect(process.stdout, emitsInOrder(["Hello from exec", emitsDone])); await process.shouldExit(0); }); @@ -650,22 +650,22 @@ void main() { .validate(); }); - test("generates ESM files if jsEsmExports is set", () async { + test("generates loadable ESM files if jsEsmExports is set", () async { await d.package(pubspec, """ void main(List args) { - pkg.jsModuleMainLibrary.value = "lib/src/module_main.dart"; + pkg.jsModuleMainLibrary.value = "lib/src/exports.dart"; pkg.jsRequires.value = [ pkg.JSRequire('util', target: pkg.JSRequireTarget.cli), - pkg.JSRequire('other', target: pkg.JSRequireTarget.node), + pkg.JSRequire('os', target: pkg.JSRequireTarget.node), ]; - pkg.jsEsmExports.value = {}; + pkg.jsEsmExports.value = {'hello'}; pkg.addNpmTasks(); grind(args); } """, [ _packageJson, - d.dir("lib/src", [d.file("module_main.dart", "void main() {}")]) + d.dir("lib/src", [_exportsHello('osLoaded')]) ]).create(); await (await grind(["pkg-npm-dev"])).shouldExit(); @@ -685,6 +685,40 @@ void main() { } }))) .validate(); + + await d.dir("depender", [ + d.file( + "package.json", + json.encode({ + "dependencies": {"my_app": "file:../my_app/build/npm"} + })), + d.file("test.mjs", """ + import * as myApp from "my_app"; + + console.log(myApp.hello); + """), + d.file("test.cjs", """ + const myApp = require("my_app"); + + console.log(myApp.hello); + """) + ]).create(); + + await (await TestProcess.start("npm", ["install"], + runInShell: true, workingDirectory: d.path("depender"))) + .shouldExit(0); + + var mjsProcess = + await TestProcess.start("node$dotExe", [d.path("depender/test.mjs")]); + expect(mjsProcess.stdout, emits("true")); + ; + await mjsProcess.shouldExit(0); + + var cjsProcess = + await TestProcess.start("node$dotExe", [d.path("depender/test.cjs")]); + expect(cjsProcess.stdout, emits("true")); + ; + await cjsProcess.shouldExit(0); }); test("overwrite existing string value", () async {