-
Notifications
You must be signed in to change notification settings - Fork 142
Adds es modules support and tests #126
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
Changes from 4 commits
46729a9
fec2197
0ff4aef
ad71efd
3160d50
d819465
cf03cbb
c7980ae
965b93e
1007d3e
964103a
2ee4347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| name: CI | ||
| on: pull_request | ||
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v1 | ||
| - uses: actions/setup-node@v1 | ||
| with: | ||
| node-version: "14.x" | ||
| - 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 | ||
|
|
||
| - 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,3 +2,4 @@ | |
| .github | ||
| bower.json | ||
| docs | ||
| test | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { createRequire } from "module"; | ||
| 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, | ||
| } = createRequire(import.meta.url)("../tslib.js"); | ||
orta marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| export { | ||
| __extends, | ||
| __assign, | ||
| __rest, | ||
| __decorate, | ||
| __param, | ||
| __metadata, | ||
| __awaiter, | ||
| __generator, | ||
| __exportStar, | ||
| __createBinding, | ||
| __values, | ||
| __read, | ||
| __spread, | ||
| __spreadArrays, | ||
| __await, | ||
| __asyncGenerator, | ||
| __asyncDelegator, | ||
| __asyncValues, | ||
| __makeTemplateObject, | ||
| __importStar, | ||
| __importDefault, | ||
| __classPrivateFieldGet, | ||
| __classPrivateFieldSet, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "type": "module" | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern definitely works if the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://nodejs.org/api/packages.html#type
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 And how is an NPM package like pkg-dir expected to behave now, in it's claim to:
The above It seems the case that NOT using the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Even before node has supported
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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,5 +25,12 @@ | |
| "module": "tslib.es6.js", | ||
| "jsnext:main": "tslib.es6.js", | ||
| "typings": "tslib.d.ts", | ||
| "sideEffects": false | ||
| "sideEffects": false, | ||
| "exports": { | ||
| ".": { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Andarist do you have any examples of packages using the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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" | ||
| }, | ||
| "./": "./" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| package-lock.json | ||
rbuckton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| rollup-modules/output | ||
| webpack-modules/dist | ||
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| const { __awaiter } = require("tslib"); | ||
|
|
||
| const testFunction = (textToPrint) => __awaiter(void 0, void 0, void 0, function* () { | ||
| console.log(`State: ${textToPrint}`); | ||
| }); | ||
|
|
||
| testFunction("Works") | ||
orta marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "scripts": { | ||
orta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "test": "node index.js" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { __awaiter } from "tslib"; | ||
|
|
||
| export const testFunction = (textToPrint) => __awaiter(void 0, void 0, void 0, function* () { | ||
| console.log(`State: ${textToPrint}`); | ||
| }); | ||
|
|
||
| testFunction("Works") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "module", | ||
| "scripts": { | ||
| "test": "node --experimental-specifier-resolution=node index.js" | ||
orta marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "dependencies": { | ||
| "tslib": "file:..", | ||
| "rollup": "2.28.2", | ||
| "@rollup/plugin-node-resolve": "9.0.0", | ||
| "webpack": "4.44.2", | ||
| "webpack-cli": "3.3.12" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { __awaiter } from "tslib"; | ||
|
|
||
| export const testFunction = (textToPrint) => __awaiter(void 0, void 0, void 0, function* () { | ||
| console.log(`State: ${textToPrint}`); | ||
| }); | ||
|
|
||
| testFunction("Works") |
| 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 output/index.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: 'output', | ||
orta marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| format: 'cjs' | ||
| }, | ||
| plugins: [nodeResolve()] | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { __awaiter } from "tslib"; | ||
|
|
||
| export const testFunction = (textToPrint) => __awaiter(void 0, void 0, void 0, function* () { | ||
| console.log(`State: ${textToPrint}`); | ||
| }); | ||
|
|
||
| testFunction("Works") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "scripts": { | ||
| "test": "../node_modules/.bin/webpack && node dist/main.js" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module.exports = { | ||
| mode: "production", | ||
| entry: "./index" | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.