Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds es modules support and tests #126

Merged
merged 12 commits into from
Oct 6, 2020
57 changes: 57 additions & 0 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
name: CI

on: pull_request

jobs:
ci:
runs-on: ubuntu-latest

strategy:
matrix:
node-version: [10.x, 12.x, 14.x]

steps:
- uses: actions/checkout@v2

- name: Use node version ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}

- name: Setup Testing Infra
run: |
cd test
npm install

- name: "CommonJS Test"
run: |
cd test/cjs
npm run test

- name: "ES Modules Test"
run: |
cd test/esm-node-native
npm run test
if: ${{ matrix.node-version == '14.x' }}

- name: "Validate ES Modules == CommonJS"
run: |
cd test/validateModuleExportsMatchCommonJS
npm run test
if: ${{ matrix.node-version == '14.x' }}

- name: "Rollup Tree-shaking Test"
run: |
cd test/rollup-modules
npm run test

- name: "Webpack Tree-shaking Test"
run: |
cd test/webpack-modules
npm run test

- name: "Snowpack Tree-shaking Test"
run: |
cd test/snowpack-modules
npm run test
if: ${{ matrix.node-version == '14.x' }}
2 changes: 2 additions & 0 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
.github
bower.json
docs
test
.npmrc
1 change: 1 addition & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package-lock=false
51 changes: 51 additions & 0 deletions modules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import tslib from '../tslib.js';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add scripts to copy tslib.es6.js to modules/index.js?

import cjs from mjs will cause warning in Angular CLI.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanna add that I was getting an error around this line in a react application after this update:

index.js:3 Uncaught (in promise) TypeError: Cannot destructure property '__extends' of 'i(...)' as it is undefined.
    at Module.97763 (index.js:3)
    at i (bootstrap:18)
    at sharing:34
    at Object.u.<computed> (remoteEntry.js:1)
    at i (remoteEntry.js:1)
    at Module.8209 (209.2823e81d61a69038a4b4.chunk.js:2)
    at Function.i (remoteEntry.js:1)

image

Let me know if I shall create an issue for this.

Some specs:
React 16.13.0
Bundled using: Webpack 5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@udayrajMT if you could make a sample that'd be great, I have some webpack tests using modules in here but must have missed something 👍🏻

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried running it on a react-start-kit with the upgraded version, but that seems to run fine 🤔
Will check my configuration again and let you know if I find a way to reproduce the issue in a standalone app

const {
orta marked this conversation as resolved.
Show resolved Hide resolved
__extends,
__assign,
__rest,
__decorate,
__param,
__metadata,
__awaiter,
__generator,
__exportStar,
__createBinding,
__values,
__read,
__spread,
__spreadArrays,
__await,
__asyncGenerator,
__asyncDelegator,
__asyncValues,
__makeTemplateObject,
__importStar,
__importDefault,
__classPrivateFieldGet,
__classPrivateFieldSet,
} = tslib;
export {
__extends,
__assign,
__rest,
__decorate,
__param,
__metadata,
__awaiter,
__generator,
__exportStar,
__createBinding,
__values,
__read,
__spread,
__spreadArrays,
__await,
__asyncGenerator,
__asyncDelegator,
__asyncValues,
__makeTemplateObject,
__importStar,
__importDefault,
__classPrivateFieldGet,
__classPrivateFieldSet,
};
3 changes: 3 additions & 0 deletions modules/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "module"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern definitely works if the .js extension is important for both. I take it that is the constraint you're working too, even if .mjs would be slightly simpler.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what was the point of adding this almost empty package.json with no package name, no package version?

Sincerely,
ES Module Bundlers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://nodejs.org/api/packages.html#type

The "type" field defines the module format that Node.js uses for all .js files that have that package.json file as their nearest parent.

Copy link

@Sweetog Sweetog Jun 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is it best to comprehensively interpret this spec like this though? To just drop a nested, nearly empty package.json in a distribution directory, when thinking about all the nuances that go into an NPM package and bundling/transpiling packages?

And how is an NPM package like pkg-dir expected to behave now, in it's claim to:

Find the root directory of a Node.js project or npm package

The above pkg-dir behavior will fail to resolve the root of the NPM package for tslib after an ESM bundler follows to path ./imports/index.js declared in package.json#exports['.'].imports and then executes pkg-dir with a cwd of: node_modules/tslib/imports/index.js

It seems the case that NOT using the .mjs extension was a shortcut and may be more the problem here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is it best to comprehensively interpret this spec like this though?

The spec is quite intentionally written like this as far as I know. It intentionally doesn't use something like "package.json at the root of the package directory" multiple times

To just drop a nested, nearly empty package.json in a distribution directory, when thinking about all the nuances that go into an NPM package and bundling/transpiling packages?

Even before node has supported .mjs, .cjs, package.json#type and more, it was already possible to define nested package.json files. It was possible to put package.json#main there to "redirect" the request to another location. I've been using this for years already.

And how is an NPM package like pkg-dir expected to behave now, in it's claim to:

I'm not sure if this package is implemented correctly but a lot of similar resolvers were implemented correctly in the past. All modern module bundlers etc already work just OK with those nested package.json files. The rule, in fact, is quite simple if you want to just grab the root of a pkg, arguably you wouldn't even have to look for package.json files but rather just find a directory nested directly within node_modules directory

9 changes: 8 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,12 @@
"module": "tslib.es6.js",
"jsnext:main": "tslib.es6.js",
"typings": "tslib.d.ts",
"sideEffects": false
"sideEffects": false,
"exports": {
".": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide "module" condition as well. It's going to be supported by the upcoming webpack 5 and will allow it to keep only a single copy of tslib in the bundle. Point of reference: https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62#reference-syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to covering this in a new PR, that gist isn't really enough docs for me to go with yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can be added afterwards.

There are no real docs for this yet on their page but this gist has been updated multiple times when they have been iterating over semantics. This is already implemented and shipped in webpack 5 RC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andarist do you have any examples of packages using the "module" condition successfully for reference here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guybedford I did not (mainly because conditional exports themselves are rather rare to find so far), but I've decided to showcase this using one of my smaller modules. Here are exports that I have defined:
https://github.com/Andarist/callbag-last-element/blob/07099fd55b4dfff80a7697065c71ce9b5fc07ade/package.json#L7-L14
and here is the webpack output:
https://github.com/Andarist/webpack-module-condition-test/tree/7bca3a5ae4f07b5429c7b36db3d43b2072b39cf2/src
Notice how last function is only there a single time in this bundle even when mixing ESM and CJS files.

The input for this is as simple as:

// cjs-file.js
module.exports = require("callbag-last-element").default;

// index.js
import lastElement from "callbag-last-element";
import lastElementFromCjs from "./cjs-file";

console.log({ lastElement, lastElementFromCjs });

and can be found here (same repo)

"import": "./modules/index.js",
"default": "./tslib.js"
},
"./": "./"
}
}
2 changes: 2 additions & 0 deletions test/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
package-lock.json
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
*/build
4 changes: 4 additions & 0 deletions test/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Tests

Each folder has a `test` script which imports `tslib` via node_modules.
In order to run these tests, you first need to run `npm install` in this folder.
2 changes: 2 additions & 0 deletions test/cjs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
const tslib = require("tslib");
if (typeof tslib.__awaiter !== "function") throw new Error("Missing expected helper __awaiter");
6 changes: 6 additions & 0 deletions test/cjs/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"private": true,
"scripts": {
orta marked this conversation as resolved.
Show resolved Hide resolved
"test": "node index.js"
}
}
2 changes: 2 additions & 0 deletions test/esm-node-native/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { __awaiter } from "tslib";
if (typeof __awaiter !== "function") throw new Error("Missing expected helper __awaiter");
6 changes: 6 additions & 0 deletions test/esm-node-native/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "module",
"scripts": {
"test": "node index.js"
}
}
10 changes: 10 additions & 0 deletions test/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"dependencies": {
"tslib": "file:..",
"rollup": "2.28.2",
"@rollup/plugin-node-resolve": "9.0.0",
"webpack": "4.44.2",
"webpack-cli": "3.3.12",
"snowpack": "^2.12.1"
}
}
2 changes: 2 additions & 0 deletions test/rollup-modules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { __awaiter } from "tslib";
if (typeof __awaiter !== "function") throw new Error("Missing expected helper __awaiter");
5 changes: 5 additions & 0 deletions test/rollup-modules/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"scripts": {
"test": "../node_modules/.bin/rollup -c rollup.config.js && node build/index.js"
}
}
10 changes: 10 additions & 0 deletions test/rollup-modules/rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { nodeResolve } from '@rollup/plugin-node-resolve';

export default {
input: 'index.js',
output: {
dir: 'build',
format: 'cjs'
},
plugins: [nodeResolve()]
};
2 changes: 2 additions & 0 deletions test/snowpack-modules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { __awaiter } from "tslib";
if (typeof __awaiter !== "function") throw new Error("Missing expected helper __awaiter");
7 changes: 7 additions & 0 deletions test/snowpack-modules/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "module",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required, but I'd be tempted to say you should move the "dependencies" from https://github.com/microsoft/tslib/pull/126/files#diff-f1e4194bc14f4a4ca169da388d9135de to each individual test so they run in isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, normally this is something I'd just throw into a yarn workspace and forget, but I'll make them all conform to a similar scripts structure then just loop through them

"scripts": {
"test": "../node_modules/.bin/snowpack build; node build/index.js"
}
}
23 changes: 23 additions & 0 deletions test/validateModuleExportsMatchCommonJS/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// When on node 14, it validates that all of the commonjs exports
// are correctly re-exported for es modules importers.

const nodeMajor = Number(process.version.split(".")[0].slice(1))
if (nodeMajor < 14) {
console.log("Skipping because node does not support module exports.")
process.exit(0)
}

// ES Modules import via the ./modules folder
import * as esTSLib from "../../modules/index.js"

// Force a commonjs resolve
import { createRequire } from "module";
const commonJSTSLib = createRequire(import.meta.url)("../../tslib.js");

for (const key in commonJSTSLib) {
if (commonJSTSLib.hasOwnProperty(key)) {
if(!esTSLib[key]) throw new Error(`ESModules is missing ${key} - it needs to be re-exported in ./modules/index.js`)
}
}

console.log("All exports in commonjs are available for es module consumers.")
6 changes: 6 additions & 0 deletions test/validateModuleExportsMatchCommonJS/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "module",
"scripts": {
"test": "node index.js"
}
}
2 changes: 2 additions & 0 deletions test/webpack-modules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { __awaiter } from "tslib";
if (typeof __awaiter !== "function") throw new Error("Missing expected helper __awaiter");
5 changes: 5 additions & 0 deletions test/webpack-modules/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"scripts": {
"test": "../node_modules/.bin/webpack && node build/main.js"
}
}
12 changes: 12 additions & 0 deletions test/webpack-modules/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const path = require('path');

/** @type {import("webpack").Configuration} */
const config = {
mode: "production",
entry: "./index",
output: {
path: path.join(process.cwd(), 'build')
}
}

module.exports = config