Validate export structure of every entrypoint#269
Conversation
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| "@rollup/plugin-node-resolve": "9.0.0", | ||
| "webpack": "4.44.2", | ||
| "webpack-cli": "3.3.12", | ||
| "snowpack": "2.12.1", |
There was a problem hiding this comment.
snowpack is no longer being maintained
| __esDecorate: __esDecorate, | ||
| __runInitializers: __runInitializers, | ||
| __propKey: __propKey, | ||
| __setFunctionName: __setFunctionName, |
There was a problem hiding this comment.
New tests caught that these were missing
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
| 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, | ||
| }); |
There was a problem hiding this comment.
New tests noticed that this compat hint was needed for named exports to be seen by Node.js ESM
There was a problem hiding this comment.
This is redundant with new tests
There was a problem hiding this comment.
This is redundant with new tests
There was a problem hiding this comment.
This is redundant with new tests
There was a problem hiding this comment.
I meant to unstage this, but will use it in a follow-up PR
| "dependencies": { | ||
| "chalk": "^4.1.2", | ||
| "rollup": "4.22.0", | ||
| "tslib": "file:..", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It’s definitely a symlink in npm
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
Seems correct to be but will defer to Ron :)
2 participants
Also updates existing test dependencies and deletes some old versions and deprecated bundlers from the test scenarios.