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

chore!: fix build:cjs run script, drop node 12 #471

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

shadowspawn
Copy link
Member

@shadowspawn shadowspawn commented Mar 4, 2023

The build is broken (#470), possibly by interaction of ts-transform-default-export and rollup-plugin-ts.

I tried upgrading rollup and failing to get the build working, stuck at new error:

[!] (plugin Typescript) TypeError: transformDefaultExport is not a function

However, ts-transform-default-export may no longer be required!

Installing yargs-parser (currently v21.1.1) the export in build/build.cjs looks like:

module.exports = yargsParser;

Removing ts-transform-default-export, the export in build/build.cjs looks as desired:

module.exports = yargsParser;

I see that rollup has a related configuration option, which we could perhaps set explicitly to reduce chance it changes in future: https://rollupjs.org/configuration-options/#output-exports
Update: already have output.exports = 'default'.

Fixes: #470

(Opening this PR as a draft since I hacked my way to a working build without deep knowledge of rollup et al. Feedback from gentle readers on things to check whether this is an adequate solution are welcome!)

@shadowspawn
Copy link
Member Author

Cough: well, that broke all the CI! Some reading to do...

@shadowspawn
Copy link
Member Author

shadowspawn commented Mar 4, 2023

Bump the minimum node version for engine, and tests. Pinned back rollup-plugin-ts after getting errors on node 14.

Update, rollup-plugin-ts v3.2.0 dropped support for node 14 (oops!) : wessberg/rollup-plugin-ts#202

@shadowspawn
Copy link
Member Author

The official rollup typescript plugin appears to work fine (tests pass) and supports "node": ">=14.0.0", so will try switching to that.

https://www.npmjs.com/package/@rollup/plugin-typescript

@shadowspawn
Copy link
Member Author

shadowspawn commented Mar 4, 2023

Coverage dropped slightly and failed CI check, presumably because some extra code included by different typescript plugin. I have not been brave enough to try a diff on the output yet, but now might be the time...
Edit: generating identical cjs code, apart from sourceMap comment for "test" build.

Note to self: run more of the checks before pushing! I am used to editor catching the lint problems...

@shadowspawn
Copy link
Member Author

shadowspawn commented Mar 4, 2023

Some interaction between tsconfig sourceMap and rollup sourcemap and plugin configuration meant maps were being generated. Added an explicit passthrough of the local configured setting in rollup configuration.

Edit: removed override. Generating sourcemap is expected for test due to env?

    "pretest": "rimraf build && tsc -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs",

@shadowspawn shadowspawn changed the title chore: fix build:cjs run script chore!: fix build:cjs run script, drop node 12 Mar 5, 2023
@tommy-mitchell
Copy link

Pulled this branch down locally and it's working for me to compile and run tests.

Rollup supports setting environment variables through its CLI, meaning we can get rid of cross-env:

- "pretest": "rimraf build && tsc -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs",
+ "pretest": "rimraf build && tsc -p tsconfig.test.json && npm run build:cjs -- --environment NODE_ENV:test",

This could also be simplified to just a TEST environment variable:

// package.json
- npm run build:cjs -- --environment NODE_ENV:test
+ npm run build:cjs -- --environment TEST

// rollup.config.js
- if (process.env.NODE_ENV === 'test') output.sourcemap = true
+ if (process.env.TEST === 'true') output.sourcemap = true

@shadowspawn
Copy link
Member Author

Thanks @tommy-mitchell. I will look into those suggestions.

@shadowspawn
Copy link
Member Author

I like the --environment approach to drop a dependency and use the rollup support.

(The pattern of passing options into another run-script is a little unfamiliar to me, but I am coming around to the idea.)

@shadowspawn
Copy link
Member Author

Reminded by the failure that the coverage fails because it is being run against the test build which adds a last line to file:

//# sourceMappingURL=index.cjs.map

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken build on fresh clone (developer)
2 participants