-
Notifications
You must be signed in to change notification settings - Fork 313
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
build a bundle using vite #1380
Conversation
Wow, this is great! Excited to try it out. I will have to educate myself a bit on the vite ecosystem, as I'm not as familiar with it. One thing I am anticipating is switching from CommonJS to ESM (in progress: #1358). What else is involved with converting to ESM with the new vite build? I am assuming it will be pretty straightforward since vite is esm-based.
https://vitejs.dev/guide/build.html#library-mode Thoughts? Vite itself seems to recommend using rollup or esbuild for non-browser projects. |
For esm, replace The lib mode is meant for browsers but it seems to work fine for node as well when you add |
Adding to the vite lib thing, vite also has vitest and node-vite which are very nice indeed, and using the other tools they recommend is a lot more work to set up. I think they were just really focused on the best browser/framework builds, but this "niche" target works well too in my experience. |
I've used vitest before and I'm a big fan. Would love to replace the old mocha tests eventually. I'm less clear on the advantages of vite-node, but ts-node is terribly slow, so I'm sure it's better than that. As for rollup/esbuild, I'm happy to use vite-node instead if it works. I'm just happy to have the contribution. |
302c02f
to
50d4b93
Compare
Aww, looks like typescript-json-schema uses ts-node (which is why it was still installed and working), and mocha needs it for .ts files, so we can't remove it. But when you migrate to vitest, at least it can go from package.json again, and maybe at some point typescript-json-schema will use esbuild somehow. So I reinstated it |
typescript-json-schema includes ts-node as a dependency itself. We shouldn't have to include it as a dependency in npm-check-updates for typescript-json-schema 馃 But if mocha still needs it, then we'll have to keep it for now. |
no, the dependency is for mocha. I don't like relying on the flat package layout... |
There are a couple issues with the build:
|
25%, wow!
|
At the moment I am seeing 5 chunks, which doesn't make sense to me, but I guess we can leave it as is. I just want to make sure it's not doing any browser-based or ssr optimizations that are unnecessary for a cli.
I'm not sure why I guess we just need to test that both CJS and ESM can import the new bundle correctly. I don't have integration tests for the bundle unfortunately. I have seen some creative ways to test module imports, but I have yet to see a really simple solution that provides test coverage. |
As far as I can tell, the build is fine. At first I didn't have the SSR:true bit and it failed for chalk because it was using the browser imports, so I'm pretty confident it's all good now. For testing esm and cjs, that might be as simple as two .cjs and .mjs files that import and run some null operation? |
Great, let's go with it.
Yes, that's right. However, we need to import something close to the tarball that gets published to npm rather than straight from the source code. Otherwise the entry points, included files, and exports won't be realistically tested. In truth, this is something I should add to the test suite in preparation for the next major version release anyway in order to mitigate the risks that come with upgrading to esm. I was able to avoid it for a long time because the build system and module system of npm-check-updates have not changed in many years. Given this PR and the planned move to ESM, now is a good time to add some better e2e module testing. I did a little bit of research and it seems there are two main approaches:
I prefer the second approach for npm-check-updates since we do not have a large monorepo with various packages and peer dependencies that need to be tested in concert. package-preview takes the Let me know if you're interested in working on the e2e module tests, otherwise we can pause this PR until I have some time to work on it on my own. |
Ok, we can do the esm now? And pnpm is very nice :) |
I'm not sure what you mean exactly. Can you clarify? To make any changes to the build output, we need e2e tests. Otherwise there is a risk that the change could break something in between building, publishing, and importing modules/types.
Yes, I use pnpm on some other projects. Since npm-check-updates works closely with npm, I've decided to stick with npm rather than use a third party package manager on this project. |
So I can do the e2e test with pnpm conversion of you like, and I can make your build ESM (but I tried to do both before and the ESM build was twice as big so maybe not?) |
Do you mean converting npm-check-updates to use pnpm, or converting package-preview to use npm? I have no intention to add pnpm to npm-check-updates itself, although having it as a nested devDependency is acceptable in the short term. I was thinking we just use package-preview as-is, or something similar, to reduce work. Maybe in the future I will fork it so it works without the internal pnpm dependency.
CJS to ESM conversion is currently in progress in #1358. I'm not sure how much of that will need to get reworked after this PR, but let's keep the ESM work separate from the vite bundle work. Both this PR and #1358 depend on good e2e tests before being merged. The e2e tests should include a test that imports npm-check-updates into both a CJS project and an ESM project (i.e. |
Package-preview adds the package to node_modules with pnpm so it rewrites the tree. Afterwards, npm might be confused. If that's ok, I can do the e2e. |
Ah, I see. I didn't realize it would rewrite the dependency tree. Let's look for a solution that is compatible with npm's node_modules. Maybe there is a different lib, or maybe package-preview can be forked. No pressure of course. I can work on it when I get some more time. |
E2E testing: #1386 |
@wmertens E2E tests are set up and added to the CI. You can run Please merge/rebase at your convenience. |
Odd that it fails on node v20 but passes on v18 馃 Update: I wonder if node-gyp should not be bundled? |
FYI, vite supports rollup plugins. For node externals, please use rollup-plugin-node-externals |
ah - the problem is that it's doing a CJS require of node-gyp and vite doesn't support that. So then node-gyp needs to be a dep |
84e73bc
to
dc93642
Compare
Great, looks good. I just want to check in about the package size, which increased from 134.9 kB to 2.2 MB. Is this expected? Is this explained by the fact that all the dependencies are part of the bundle, and are not downloaded separately during npm install? Also, does rollup automatically exclude actual devDependencies from the build?
$ npm pack --verbose --dry-run
npm info using npm@10.2.4
npm info using node@v20.11.0
npm verb title npm pack
npm verb argv "pack" "--loglevel" "verbose" "--dry-run"
npm verb logfile logs-max:10 dir:/Users/raine/.npm/_logs/2024-03-24T22_25_08_051Z-
npm verb logfile /Users/raine/.npm/_logs/2024-03-24T22_25_08_051Z-debug-0.log
npm notice
npm notice 馃摝 npm-check-updates@17.0.0-2
npm notice === Tarball Contents ===
npm notice 555B LICENSE
npm notice 28.0kB README.md
npm notice 6.0kB build/package.json
npm notice 31B build/src/bin/cli.d.ts
npm notice 10.4kB build/src/bin/cli.js
npm notice 7.3kB build/src/bin/cli.js.map
npm notice 427B build/src/cli-options.d.ts
npm notice 33.6kB build/src/cli-options.js
npm notice 18.6kB build/src/cli-options.js.map
npm notice 651B build/src/index.d.ts
npm notice 16.2kB build/src/index.js
npm notice 9.5kB build/src/index.js.map
npm notice 816B build/src/lib/cache.d.ts
npm notice 4.0kB build/src/lib/cache.js
npm notice 3.1kB build/src/lib/cache.js.map
npm notice 401B build/src/lib/chalk.d.ts
npm notice 2.4kB build/src/lib/chalk.js
npm notice 1.6kB build/src/lib/chalk.js.map
npm notice 421B build/src/lib/determinePackageManager.d.ts
npm notice 1.2kB build/src/lib/determinePackageManager.js
npm notice 776B build/src/lib/determinePackageManager.js.map
npm notice 451B build/src/lib/doctor.d.ts
npm notice 13.3kB build/src/lib/doctor.js
npm notice 8.6kB build/src/lib/doctor.js.map
npm notice 119B build/src/lib/exists.d.ts
npm notice 460B build/src/lib/exists.js
npm notice 313B build/src/lib/exists.js.map
npm notice 107B build/src/lib/figgy-pudding/index.d.ts
npm notice 3.1kB build/src/lib/figgy-pudding/index.js
npm notice 3.7kB build/src/lib/figgy-pudding/index.js.map
npm notice 545B build/src/lib/filterAndReject.d.ts
npm notice 3.6kB build/src/lib/filterAndReject.js
npm notice 2.5kB build/src/lib/filterAndReject.js.map
npm notice 221B build/src/lib/filterObject.d.ts
npm notice 532B build/src/lib/filterObject.js
npm notice 416B build/src/lib/filterObject.js.map
npm notice 572B build/src/lib/findLockfile.d.ts
npm notice 2.3kB build/src/lib/findLockfile.js
npm notice 1.5kB build/src/lib/findLockfile.js.map
npm notice 356B build/src/lib/findPackage.d.ts
npm notice 3.5kB build/src/lib/findPackage.js
npm notice 2.5kB build/src/lib/findPackage.js.map
npm notice 482B build/src/lib/getAllPackages.d.ts
npm notice 6.5kB build/src/lib/getAllPackages.js
npm notice 4.3kB build/src/lib/getAllPackages.js.map
npm notice 617B build/src/lib/getCurrentDependencies.d.ts
npm notice 3.7kB build/src/lib/getCurrentDependencies.js
npm notice 1.7kB build/src/lib/getCurrentDependencies.js.map
npm notice 560B build/src/lib/getIgnoredUpgrades.d.ts
npm notice 1.6kB build/src/lib/getIgnoredUpgrades.js
npm notice 1.3kB build/src/lib/getIgnoredUpgrades.js.map
npm notice 375B build/src/lib/getInstalledPackages.d.ts
npm notice 1.5kB build/src/lib/getInstalledPackages.js
npm notice 896B build/src/lib/getInstalledPackages.js.map
npm notice 493B build/src/lib/getNcuRc.d.ts
npm notice 3.1kB build/src/lib/getNcuRc.js
npm notice 2.3kB build/src/lib/getNcuRc.js.map
npm notice 538B build/src/lib/getPackageManager.d.ts
npm notice 1.2kB build/src/lib/getPackageManager.js
npm notice 602B build/src/lib/getPackageManager.js.map
npm notice 591B build/src/lib/getPeerDependenciesFromRegistry.d.ts
npm notice 2.7kB build/src/lib/getPeerDependenciesFromRegistry.js
npm notice 2.1kB build/src/lib/getPeerDependenciesFromRegistry.js.map
npm notice 385B build/src/lib/getPreferredWildcard.d.ts
npm notice 1.5kB build/src/lib/getPreferredWildcard.js
npm notice 997B build/src/lib/getPreferredWildcard.js.map
npm notice 592B build/src/lib/getRepoUrl.d.ts
npm notice 3.3kB build/src/lib/getRepoUrl.js
npm notice 2.3kB build/src/lib/getRepoUrl.js.map
npm notice 308B build/src/lib/initOptions.d.ts
npm notice 11.3kB build/src/lib/initOptions.js
npm notice 8.0kB build/src/lib/initOptions.js.map
npm notice 444B build/src/lib/isUpgradeable.d.ts
npm notice 3.5kB build/src/lib/isUpgradeable.js
npm notice 1.2kB build/src/lib/isUpgradeable.js.map
npm notice 488B build/src/lib/keyValueBy.d.ts
npm notice 1.1kB build/src/lib/keyValueBy.js
npm notice 896B build/src/lib/keyValueBy.js.map
npm notice 120B build/src/lib/libnpmconfig/index.d.ts
npm notice 3.5kB build/src/lib/libnpmconfig/index.js
npm notice 3.6kB build/src/lib/libnpmconfig/index.js.map
npm notice 275B build/src/lib/loadPackageInfoFromFile.d.ts
npm notice 942B build/src/lib/loadPackageInfoFromFile.js
npm notice 655B build/src/lib/loadPackageInfoFromFile.js.map
npm notice 3.1kB build/src/lib/logging.d.ts
npm notice 10.5kB build/src/lib/logging.js
npm notice 8.2kB build/src/lib/logging.js.map
npm notice 3.2kB build/src/lib/mergeOptions.d.ts
npm notice 830B build/src/lib/mergeOptions.js
npm notice 875B build/src/lib/mergeOptions.js.map
npm notice 241B build/src/lib/programError.d.ts
npm notice 717B build/src/lib/programError.js
npm notice 552B build/src/lib/programError.js.map
npm notice 726B build/src/lib/queryVersions.d.ts
npm notice 6.1kB build/src/lib/queryVersions.js
npm notice 4.2kB build/src/lib/queryVersions.js.map
npm notice 237B build/src/lib/resolveDepSections.d.ts
npm notice 864B build/src/lib/resolveDepSections.js
npm notice 713B build/src/lib/resolveDepSections.js.map
npm notice 239B build/src/lib/runGlobal.d.ts
npm notice 3.4kB build/src/lib/runGlobal.js
npm notice 2.5kB build/src/lib/runGlobal.js.map
npm notice 877B build/src/lib/runLocal.d.ts
npm notice 11.5kB build/src/lib/runLocal.js
npm notice 9.1kB build/src/lib/runLocal.js.map
npm notice 248B build/src/lib/table.d.ts
npm notice 1.1kB build/src/lib/table.js
npm notice 1.0kB build/src/lib/table.js.map
npm notice 694B build/src/lib/upgradeDependencies.d.ts
npm notice 5.5kB build/src/lib/upgradeDependencies.js
npm notice 3.0kB build/src/lib/upgradeDependencies.js.map
npm notice 656B build/src/lib/upgradePackageData.d.ts
npm notice 2.3kB build/src/lib/upgradePackageData.js
npm notice 1.6kB build/src/lib/upgradePackageData.js.map
npm notice 605B build/src/lib/upgradePackageDefinitions.d.ts
npm notice 3.4kB build/src/lib/upgradePackageDefinitions.js
npm notice 2.0kB build/src/lib/upgradePackageDefinitions.js.map
npm notice 6.5kB build/src/lib/version-util.d.ts
npm notice 22.5kB build/src/lib/version-util.js
npm notice 14.8kB build/src/lib/version-util.js.map
npm notice 192B build/src/lib/wrap.d.ts
npm notice 1.7kB build/src/lib/wrap.js
npm notice 1.3kB build/src/lib/wrap.js.map
npm notice 1.1kB build/src/package-managers/bun.d.ts
npm notice 4.3kB build/src/package-managers/bun.js
npm notice 2.6kB build/src/package-managers/bun.js.map
npm notice 2.0kB build/src/package-managers/filters.d.ts
npm notice 4.6kB build/src/package-managers/filters.js
npm notice 1.7kB build/src/package-managers/filters.js.map
npm notice 1.1kB build/src/package-managers/gitTags.d.ts
npm notice 5.7kB build/src/package-managers/gitTags.js
npm notice 3.7kB build/src/package-managers/gitTags.js.map
npm notice 174B build/src/package-managers/index.d.ts
npm notice 1.5kB build/src/package-managers/index.js
npm notice 308B build/src/package-managers/index.js.map
npm notice 5.4kB build/src/package-managers/npm.d.ts
npm notice 30.2kB build/src/package-managers/npm.js
npm notice 20.6kB build/src/package-managers/npm.js.map
npm notice 1.1kB build/src/package-managers/pnpm.d.ts
npm notice 5.3kB build/src/package-managers/pnpm.js
npm notice 3.0kB build/src/package-managers/pnpm.js.map
npm notice 270B build/src/package-managers/staticRegistry.d.ts
npm notice 1.8kB build/src/package-managers/staticRegistry.js
npm notice 1.2kB build/src/package-managers/staticRegistry.js.map
npm notice 2.3kB build/src/package-managers/yarn.d.ts
npm notice 11.6kB build/src/package-managers/yarn.js
npm notice 7.2kB build/src/package-managers/yarn.js.map
npm notice 11B build/src/scripts/build-options.d.ts
npm notice 6.3kB build/src/scripts/build-options.js
npm notice 3.8kB build/src/scripts/build-options.js.map
npm notice 297B build/src/types/Cacher.d.ts
npm notice 111B build/src/types/Cacher.js
npm notice 116B build/src/types/Cacher.js.map
npm notice 487B build/src/types/CLIOption.d.ts
npm notice 114B build/src/types/CLIOption.js
npm notice 122B build/src/types/CLIOption.js.map
npm notice 170B build/src/types/ExtendedHelp.d.ts
npm notice 117B build/src/types/ExtendedHelp.js
npm notice 128B build/src/types/ExtendedHelp.js.map
npm notice 191B build/src/types/FilterFunction.d.ts
npm notice 119B build/src/types/FilterFunction.js
npm notice 132B build/src/types/FilterFunction.js.map
npm notice 200B build/src/types/FilterPattern.d.ts
npm notice 118B build/src/types/FilterPattern.js
npm notice 130B build/src/types/FilterPattern.js.map
npm notice 350B build/src/types/FilterResultsFunction.d.ts
npm notice 126B build/src/types/FilterResultsFunction.js
npm notice 146B build/src/types/FilterResultsFunction.js.map
npm notice 397B build/src/types/GetVersion.d.ts
npm notice 115B build/src/types/GetVersion.js
npm notice 124B build/src/types/GetVersion.js.map
npm notice 412B build/src/types/GroupFunction.d.ts
npm notice 118B build/src/types/GroupFunction.js
npm notice 130B build/src/types/GroupFunction.js.map
npm notice 258B build/src/types/IgnoredUpgrade.d.ts
npm notice 119B build/src/types/IgnoredUpgrade.js
npm notice 132B build/src/types/IgnoredUpgrade.js.map
npm notice 86B build/src/types/IndexType.d.ts
npm notice 199B build/src/types/IndexType.js
npm notice 134B build/src/types/IndexType.js.map
npm notice 97B build/src/types/Maybe.d.ts
npm notice 110B build/src/types/Maybe.js
npm notice 114B build/src/types/Maybe.js.map
npm notice 332B build/src/types/MockedVersions.d.ts
npm notice 119B build/src/types/MockedVersions.js
npm notice 132B build/src/types/MockedVersions.js.map
npm notice 151B build/src/types/NpmConfig.d.ts
npm notice 114B build/src/types/NpmConfig.js
npm notice 122B build/src/types/NpmConfig.js.map
npm notice 142B build/src/types/NpmOptions.d.ts
npm notice 115B build/src/types/NpmOptions.js
npm notice 124B build/src/types/NpmOptions.js.map
npm notice 688B build/src/types/Options.d.ts
npm notice 112B build/src/types/Options.js
npm notice 118B build/src/types/Options.js.map
npm notice 798B build/src/types/PackageFile.d.ts
npm notice 116B build/src/types/PackageFile.js
npm notice 126B build/src/types/PackageFile.js.map
npm notice 140B build/src/types/PackageFileRepository.d.ts
npm notice 126B build/src/types/PackageFileRepository.js
npm notice 146B build/src/types/PackageFileRepository.js.map
npm notice 211B build/src/types/PackageInfo.d.ts
npm notice 116B build/src/types/PackageInfo.js
npm notice 126B build/src/types/PackageInfo.js.map
npm notice 888B build/src/types/PackageManager.d.ts
npm notice 119B build/src/types/PackageManager.js
npm notice 132B build/src/types/PackageManager.js.map
npm notice 126B build/src/types/PackageManagerName.d.ts
npm notice 123B build/src/types/PackageManagerName.js
npm notice 140B build/src/types/PackageManagerName.js.map
npm notice 415B build/src/types/Packument.d.ts
npm notice 114B build/src/types/Packument.js
npm notice 122B build/src/types/Packument.js.map
npm notice 7.9kB build/src/types/RunOptions.d.ts
npm notice 115B build/src/types/RunOptions.js
npm notice 124B build/src/types/RunOptions.js.map
npm notice 157B build/src/types/SpawnOptions.d.ts
npm notice 117B build/src/types/SpawnOptions.js
npm notice 128B build/src/types/SpawnOptions.js.map
npm notice 157B build/src/types/SpawnPleaseOptions.d.ts
npm notice 123B build/src/types/SpawnPleaseOptions.js
npm notice 140B build/src/types/SpawnPleaseOptions.js.map
npm notice 99B build/src/types/StaticRegistry.d.ts
npm notice 119B build/src/types/StaticRegistry.js
npm notice 132B build/src/types/StaticRegistry.js.map
npm notice 705B build/src/types/Target.d.ts
npm notice 340B build/src/types/Target.js
npm notice 223B build/src/types/Target.js.map
npm notice 209B build/src/types/TargetFunction.d.ts
npm notice 119B build/src/types/TargetFunction.js
npm notice 132B build/src/types/TargetFunction.js.map
npm notice 86B build/src/types/UpgradeGroup.d.ts
npm notice 117B build/src/types/UpgradeGroup.js
npm notice 128B build/src/types/UpgradeGroup.js.map
npm notice 134B build/src/types/Version.d.ts
npm notice 112B build/src/types/Version.js
npm notice 118B build/src/types/Version.js.map
npm notice 105B build/src/types/VersionLevel.d.ts
npm notice 117B build/src/types/VersionLevel.js
npm notice 128B build/src/types/VersionLevel.js.map
npm notice 300B build/src/types/VersionResult.d.ts
npm notice 118B build/src/types/VersionResult.js
npm notice 130B build/src/types/VersionResult.js.map
npm notice 167B build/src/types/VersionSpec.d.ts
npm notice 116B build/src/types/VersionSpec.js
npm notice 126B build/src/types/VersionSpec.js.map
npm notice 5.3kB package.json
npm notice === Tarball Details ===
npm notice name: npm-check-updates
npm notice version: 17.0.0-2
npm notice filename: npm-check-updates-17.0.0-2.tgz
npm notice package size: 134.9 kB
npm notice unpacked size: 574.3 kB
npm notice shasum: 54bcd13e951862347600be284e346364c3d9147e
npm notice integrity: sha512-C58pUX/Szm13w[...]RTnVXKceD616A==
npm notice total files: 247
npm notice
npm-check-updates-17.0.0-2.tgz
npm verb exit 0
npm info ok This branch (2.2 MB): $ npm pack --verbose --dry-run
npm info using npm@10.2.4
npm info using node@v20.11.0
npm verb title npm pack
npm verb argv "pack" "--loglevel" "verbose" "--dry-run"
npm verb logfile logs-max:10 dir:/Users/raine/.npm/_logs/2024-03-24T22_26_05_523Z-
npm verb logfile /Users/raine/.npm/_logs/2024-03-24T22_26_05_523Z-debug-0.log
npm notice
npm notice 馃摝 npm-check-updates@17.0.0-2
npm notice === Tarball Contents ===
npm notice 555B LICENSE
npm notice 28.0kB README.md
npm notice 12B build/cli.d.ts
npm notice 42.2kB build/cli.js
npm notice 165.4kB build/cli.js.map
npm notice 1.1kB build/index-_DliZFxU.js
npm notice 6.1kB build/index-_DliZFxU.js.map
npm notice 230.1kB build/index-5sFb3Tvv.js
npm notice 836.7kB build/index-5sFb3Tvv.js.map
npm notice 7.2kB build/index-BmUFwMVL.js
npm notice 29.2kB build/index-BmUFwMVL.js.map
npm notice 551B build/index-BnIU43YD.js
npm notice 1.4kB build/index-BnIU43YD.js.map
npm notice 10.4kB build/index.d.ts
npm notice 1.6MB build/index.js
npm notice 5.4MB build/index.js.map
npm notice 5.5kB package.json
npm notice === Tarball Details ===
npm notice name: npm-check-updates
npm notice version: 17.0.0-2
npm notice filename: npm-check-updates-17.0.0-2.tgz
npm notice package size: 2.2 MB
npm notice unpacked size: 8.3 MB
npm notice shasum: 8fc0db10324fcf1066ac359e3d9b711ed33c09c5
npm notice integrity: sha512-68Iyb82NQqjkE[...]PPdE//lin4kNg==
npm notice total files: 17
npm notice
npm-check-updates-17.0.0-2.tgz
npm verb exit 0
npm info ok |
Yes the increase is from bundling everything, and I suppose everything is bundled although I didn't specifically ask it to. Since the e2e tests pass I assume it bundled all it needs :) |
Yes, though that doesn't tell us if it is bundling more than it needs. How does rollup know to exclude the actual devDependencies from the bundle now that they are not separated out in the package.json? Are you sure we don't need to mark them as external? |
AFAIK, in rollup you need to specify every used dependency inside the code as external or else it will be packed as well. You only specified the node built ins as external. |
- speeds up install because there's no longer 301 dependencies - speeds up run because there's less code to parse
Sorry it was late and I missed that I had specified If the size of the package should be smaller, we can remove the sourcemaps or remove the original code from the sourcemaps. |
BTW I also tried to do an ESM build but it still contains the require.resolve from @npmcli/run-script so that won't work. That first needs to be converted to ESM before ncu can be ESM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some of the actual devDependencies to external
and it did not change the bundle size, so that seems to confirm that the tree-shaking is working.
Good to go!
|
Note that the tests now run in 4:30 vs 5:34 馃帀