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

Validate export structure of every entrypoint #269

Merged
merged 1 commit into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,14 @@ jobs:
ci:
runs-on: ubuntu-latest

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

steps:
- uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2

- name: Use node version ${{ matrix.node-version }}
- name: Setup Node.js
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version: ${{ matrix.node-version }}
node-version: 22.x
Comment on lines -14 to +18
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw very little value in running tests on older Node.js versions since the vast majority of what we’re running is just infrastructure. I changed this because I’m using Set.prototype.symmetricDifference (v22+), as well as --expeimental-detect-module (v20+ I think?) in order to load the otherwise-Node.js-invalid tslib.es6.js.

Copy link
Member

Choose a reason for hiding this comment

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

The only gotcha is that using --experimental-detect-module means we won't know if .es6 or whatever accidentally change to CJS, but I don't see any other way in node to load the wrong kind of file format. Other than to instead do these checks in playwright and use a real browser?


- name: Run tests
run: node ./test/runTests.js

- name: Run tests
run: node ./test/validateModuleExportsMatchCommonJS/index.js
if: matrix.node-version == '16.x'
2 changes: 0 additions & 2 deletions test/cjs/index.js

This file was deleted.

6 changes: 0 additions & 6 deletions test/cjs/package.json
Copy link
Member Author

Choose a reason for hiding this comment

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

This is redundant with new tests

This file was deleted.

9 changes: 0 additions & 9 deletions test/esm-node-native/package.json
Copy link
Member Author

Choose a reason for hiding this comment

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

This is redundant with new tests

This file was deleted.

38 changes: 38 additions & 0 deletions test/node/exportStructure.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import assert from "node:assert";
import { test } from "node:test";

import * as tslibJs from "tslib/tslib.js";
import * as tslibEs6Js from "tslib/tslib.es6.js";
import * as tslibEs6Mjs from "tslib/tslib.es6.mjs";
import * as tslibModulesIndex from "tslib/modules/index.js";

test("all helpers available as named exports", () => {
compareKeys("tslib.js", tslibJs, "tslib.es6.js", tslibEs6Js);
compareKeys("tslib.js", tslibJs, "tslib.es6.mjs", tslibEs6Mjs);
compareKeys("tslib.js", tslibJs, "modules/index.js", tslibModulesIndex);
});

test("default export contains all named exports", () => {
assert.ok(tslibJs.default);
compareKeys("tslib.js (default export)", tslibJs.default, "tslib.js (namespace)", tslibJs);
assert.ok(tslibEs6Js.default);
compareKeys("tslib.es6.js (default export)", tslibEs6Js.default, "tslib.es6.js (namespace)", tslibEs6Js);
assert.ok(tslibEs6Mjs.default);
compareKeys("tslib.es6.mjs (default export)", tslibEs6Mjs.default, "tslib.es6.mjs (namespace)", tslibEs6Mjs);
});

/**
* @param {string} name1
* @param {object} tslib1
* @param {string} name2
* @param {object} tslib2
*/
function compareKeys(name1, tslib1, name2, tslib2) {
const difference = new Set(Object.keys(tslib1)).symmetricDifference(new Set(Object.keys(tslib2)));
difference.delete("__esModule");
difference.delete("default"); // Asserted separately where expected
const messages = Array.from(difference).map(missing => `'${missing}' missing in ${missing in tslib1 ? name2 : name1}`);
if (messages.length > 0) {
assert.fail(`Mismatch between ${name1} and ${name2}:\n\n ${messages.join("\n ")}\n`);
}
}
6 changes: 6 additions & 0 deletions test/node/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "module",
"scripts": {
"test": "node --test --experimental-detect-module"
}
}
24 changes: 24 additions & 0 deletions test/node/testHelper.js
Copy link
Member Author

Choose a reason for hiding this comment

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

I meant to unstage this, but will use it in a follow-up PR

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { describe } from "node:test";

import * as tslibJs from "tslib/tslib.js";
import * as tslibEs6Js from "tslib/tslib.es6.js";
import * as tslibEs6Mjs from "tslib/tslib.es6.mjs";

/**
* @template {keyof tslibJs} K
* @param {K} helperName
* @param {(helper: tslibJs[K]) => any} test
*/
export function testHelper(helperName, test) {
describe(helperName, () => {
describe("tslib.js", () => {
test(tslibJs[helperName]);
});
describe("tslib.es6.js", () => {
test(tslibEs6Js[helperName]);
});
describe("tslib.es6.mjs", () => {
test(tslibEs6Mjs[helperName]);
});
});
}
10 changes: 10 additions & 0 deletions test/node/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"compilerOptions": {
"module": "NodeNext",
"checkJs": true,
"noEmit": true,
"strict": true,
"forceConsistentCasingInFileNames": true,
"types": ["node"],
}
}
11 changes: 5 additions & 6 deletions test/package.json
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
{
"dependencies": {
"chalk": "^4.1.2",
"rollup": "4.22.0",
"tslib": "file:..",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not new in this PR but IIRC this tells the package manager to copy the package and not make a link, making iterative testing more annoying.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s definitely a symlink in npm

"rollup": "2.28.2",
"@rollup/plugin-node-resolve": "9.0.0",
"webpack": "4.44.2",
"webpack-cli": "3.3.12",
"snowpack": "2.12.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

snowpack is no longer being maintained

"vite": "2.8.0"
"vite": "5.4.6",
"webpack": "5.94.0",
"webpack-cli": "5.1.4"
}
}
3 changes: 0 additions & 3 deletions test/rollup-modules/rollup.config.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { nodeResolve } from '@rollup/plugin-node-resolve';

export default {
input: 'index.js',
output: {
dir: 'build',
format: 'cjs'
},
plugins: [nodeResolve()]
};
6 changes: 3 additions & 3 deletions test/runTests.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
// @ts-check
const { spawnSync } = require("child_process");
const fs = require("fs");
const path = require("path");
const mainVersion = Number(process.version.replace("v","").split(".")[0])

// Loop through all the folders and run `npm test`

const blocklist = ["validateModuleExportsMatchCommonJS", "node_modules"];
const filesInTest = fs.readdirSync(__dirname);
const tests = filesInTest
.filter((f) => fs.statSync(path.join(__dirname, f)).isDirectory())
.filter((f) => !blocklist.includes(f));
.filter((f) => f !== "node_modules");

// Support setting up the test node modules
if (!filesInTest.includes("node_modules")) {
Expand All @@ -18,7 +18,7 @@ if (!filesInTest.includes("node_modules")) {
console.log("Installed");
}

const chalk = require("chalk").default;
const chalk = require("chalk");
for (const test of tests) {
console.log("---> " + chalk.bold(test));

Expand Down
2 changes: 0 additions & 2 deletions test/snowpack-modules/index.js

This file was deleted.

10 changes: 0 additions & 10 deletions test/snowpack-modules/package.json

This file was deleted.

17 changes: 0 additions & 17 deletions test/validateModuleExportsMatchCommonJS/index.js

This file was deleted.

6 changes: 0 additions & 6 deletions test/validateModuleExportsMatchCommonJS/package.json
Copy link
Member Author

Choose a reason for hiding this comment

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

This is redundant with new tests

This file was deleted.

2 changes: 0 additions & 2 deletions test/webpack-4-modules/index.js

This file was deleted.

5 changes: 0 additions & 5 deletions test/webpack-4-modules/package.json

This file was deleted.

2 changes: 0 additions & 2 deletions test/webpack-5-modules/index.js

This file was deleted.

10 changes: 0 additions & 10 deletions test/webpack-5-modules/package.json

This file was deleted.

12 changes: 0 additions & 12 deletions test/webpack-5-modules/webpack.config.js

This file was deleted.

File renamed without changes.
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": "webpack && node build/main.js"
}
}
4 changes: 4 additions & 0 deletions tslib.es6.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,10 @@ export default {
__rest: __rest,
__decorate: __decorate,
__param: __param,
__esDecorate: __esDecorate,
__runInitializers: __runInitializers,
__propKey: __propKey,
__setFunctionName: __setFunctionName,
Copy link
Member Author

Choose a reason for hiding this comment

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

New tests caught that these were missing

Copy link
Member

Choose a reason for hiding this comment

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

I feel like the fact that we got away without these in the .es6 files would imply that nobody's using them... Or just I guess that the overlap of new features and these old files is small...

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s not necessarily that nobody’s using these files; it’s that nobody’s using the default export from these files, which is only there for edge casey compat

__metadata: __metadata,
__awaiter: __awaiter,
__generator: __generator,
Expand Down
4 changes: 4 additions & 0 deletions tslib.es6.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ export default {
__rest,
__decorate,
__param,
__esDecorate,
__runInitializers,
__propKey,
__setFunctionName,
__metadata,
__awaiter,
__generator,
Expand Down
34 changes: 34 additions & 0 deletions tslib.js
Original file line number Diff line number Diff line change
Expand Up @@ -427,3 +427,37 @@ var __disposeResources;
exporter("__addDisposableResource", __addDisposableResource);
exporter("__disposeResources", __disposeResources);
});

0 && (module.exports = {
__extends,
__assign,
__rest,
__decorate,
__param,
__esDecorate,
__runInitializers,
__propKey,
__setFunctionName,
__metadata,
__awaiter,
__generator,
__exportStar,
__createBinding,
__values,
__read,
__spread,
__spreadArrays,
__spreadArray,
__await,
__asyncGenerator,
__asyncDelegator,
__asyncValues,
__makeTemplateObject,
__importStar,
__importDefault,
__classPrivateFieldGet,
__classPrivateFieldSet,
__classPrivateFieldIn,
__addDisposableResource,
__disposeResources,
});
Comment on lines +431 to +463
Copy link
Member Author

Choose a reason for hiding this comment

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

New tests noticed that this compat hint was needed for named exports to be seen by Node.js ESM