fix: change the build config to bundle deps correctly, fix #111 #112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SukkaW
changed the title
SukkaW
commented
Apr 16, 2024
| @@ -31,7 +31,7 @@ | |||
| "typecheck": "tsc", | |||
| "prepublishOnly": "nr build", | |||
| "release": "bumpp && pnpm publish --no-git-checks", | |||
| "test": "vitest" | |||
| "test": "unbuild && vitest" | |||
There was a problem hiding this comment.
The new test case is against the dist, so the dist needs to be built before the test happens.
SukkaW
commented
Apr 16, 2024
| @@ -67,6 +66,7 @@ | |||
| "npm-package-arg": "^11.0.2", | |||
| "npm-registry-fetch": "^16.2.1", | |||
| "rimraf": "^5.0.5", | |||
| "semver": "^7.6.0", | |||
There was a problem hiding this comment.
semver is a CommonJS module and a dependency of both taze and taze's dependency.
Rollup has trouble handling a CommonJS external from a transitive dependency, so let's bundle semver as well.
SukkaW
commented
Apr 16, 2024
| @@ -3,7 +3,7 @@ import type { Argv } from 'yargs' | |||
| import yargs from 'yargs' | |||
| import { hideBin } from 'yargs/helpers' | |||
| import c from 'picocolors' | |||
| import { version } from '../package.json' | |||
There was a problem hiding this comment.
Since we've disabled the @rollup/plugin-json's namedExports optimization, we can't use named import here.
5 tasks
antfu
approved these changes
Apr 16, 2024
antfu
changed the title
|
Thanks for the detailed explaination! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
#111 is caused by the Rollup (
unbuilduses Rollup under the hood) incorrectly bundling@npmcli/config(and its transitive dependencycacache, which requires its ownpackage.json).The PR provides a workaround by changing how dists are bundled. A test case has also been added to ensure the CLI itself should never crash.
Linked Issues
#111
Additional context
This is a workaround that works for now. I am still investigating a better solution that solves the issue once and for all.
Here are the backgrounds behind #111 and this PR:
@npmcli/configrequirescacache, andcacacherequires its ownpackage.json. This causes@rollup/plugin-commonjsto panic. Instead of using the correct named exports generated from@rollup/plugin-json,@rollup/plugin-commonjstry to wrap the exports with its interop code, causingcacacheto fail.@rollup/plugin-commonjs'srequireReturnsDefaulttoauto, disable@rollup/plugin-json'snamedExportsoptimization.@npmcli/configalso requiressemver/function/valid. Since@npmcli/configis a CJS module,require('semver/function/valid')works just fine. However, when Rollup bundles the dist, it transformsrequire('semver/function/valid')intoimport 'semver/function/valid'which is an invalid ESM import (missing the extname, it should beimport 'semver/function/valid.js'), causing the error of npx taze@latest throws [ERR_MODULE_NOT_FOUND] #111.semveras well, preventing transitive dependency from becoming external.semverwould cause another build error, sinceunbuildincludes@types/semverto be parsed by the rollup.rollup-plugin-dts'srespectExternal.