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

Use Rollup for bundling dependencies #242

Merged
merged 16 commits into from
Aug 16, 2023

Conversation

tommy-mitchell
Copy link
Contributor

@tommy-mitchell tommy-mitchell commented Aug 1, 2023

Closes #67. Bumps dev dependencies and adds an _actualDependencies field.

Builds on prepare and outputs a bundled dependencies.js and licenses.md to the build folder:

\__ build/
	\__ licenses.md # License of each bundled dependency
	\__ dependencies.js # Vendor bundle
	\__ index.js
	\__ options.js
	\__ parser.js
	\__ utils.js
	\__ validate.js
\__ index.d.ts
\__ license
\__ package.json
\__ readme.md

@sindresorhus
Copy link
Owner

I don't want to minify the code. Debugging is important. It also doesn't save much anyway.

@sindresorhus
Copy link
Owner

How should dependencies in package.json be specified?

They need to be devDependencies, but it would be nice to somehow indicate which are used in production. We could maybe create a custom field like in package.json: {"_actualDependencies": ["..."]}.

package.json Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Contributor Author

They need to be devDependencies, but it would be nice to somehow indicate which are used in production. We could maybe create a custom field like in package.json: {"_actualDependencies": ["..."]}.

Would bundleDependencies work?

@sindresorhus
Copy link
Owner

Would bundleDependencies work?

I don't think so. It would bundle the dependencies. So we would still have double up of the code (both rolled-up and npm bundled).

@tommy-mitchell
Copy link
Contributor Author

Fair enough. I'll add a custom field. If we're not minimizing, maybe we should only bundle the dependencies?

\__ dependencies/
    \__ licenses.md
    \__ bundle.js
\__ source/
    \__ index.js
    \__ ...
\__ index.d.ts
\__ license
\__ package.json
\__ readme.md

Avoids the abbreviation problem too, lol

@sindresorhus
Copy link
Owner

I think it's easier to simply bundle everything. If we do a separate dependencies file, we need to change all the import statements.

@tommy-mitchell
Copy link
Contributor Author

Whoops, you're right, was thinking a little too fast there.

@tommy-mitchell
Copy link
Contributor Author

I'm not sure when the build script should be run. Pros and cons:

  • prepack:
    • recommended by yarn
    • runs when packages are installed from git
    • also runs when np does checks (e.g. npm pack --dry-run --json)
  • prepublishOnly:
    • only runs on npm publish
    • doesn't run until needed, but git consumers can't consume meow

@LitoMore
Copy link
Contributor

LitoMore commented Aug 4, 2023

@sindresorhus
Copy link
Owner

Either prepack or prepare. I generally prefer prepare as it also executes when running npm install while developing.

* use `build` instead of `dist`
* use `prepare` instead of `prepack`
* bundle dependencies in a vendor bundle
@tommy-mitchell
Copy link
Contributor Author

Changes:

  • Added an _actualDependencies field to package.json
  • Changed output directory to build instead of dist
  • Changed build script to use prepare instead of prepack

With Rollup, I was able to separate the dependencies into a separate file and have the imports be updated:

\__ build/
	\__ licenses.md # License of each bundled dependency
	\__ dependencies.js # Vendor bundle
	\__ index.js
	\__ options.js
	\__ parser.js
	\__ utils.js
	\__ validate.js
\__ index.d.ts
\__ license
\__ package.json
\__ readme.md

@tommy-mitchell tommy-mitchell mentioned this pull request May 23, 2023
9 tasks
@sindresorhus
Copy link
Owner

I guess it doesn't hurt, but kinda weird that each generated file includes:

import 'util';
import 'path';
import 'fs';
import 'url';
import 'node:fs';
import 'os';
import 'node:path';
import 'node:url';
import 'node:process';

package.json Outdated Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Looks pretty good

@sindresorhus
Copy link
Owner

Have you manually tested it with a CLI to make sure meow still works when published? You can run npm pack and then decompress it in a CLI project.

@tommy-mitchell
Copy link
Contributor Author

Have you manually tested it

I had before, but I went ahead and added a couple of tests using the built output to verify going forward.

@tommy-mitchell
Copy link
Contributor Author

I guess it doesn't hurt, but kinda weird that each generated file includes

Yeah I'm not sure what's up with that, I need to do some research.

test/build.js Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Contributor Author

Managed to fix the empty Node builtin imports. I think this should be good to go.

@sindresorhus sindresorhus merged commit 41e628c into sindresorhus:main Aug 16, 2023
3 checks passed
@Grohden
Copy link

Grohden commented Nov 8, 2023

@sindresorhus @tommy-mitchell
One question about this PR: How does it work with security checks and resolution bumps? Since dependencies are now bundled and not declared in the package.json, security issues with them might not be reported anymore by bots... we will also not be able to properly check if a dependency is updated

In the old version:

➜  project git:(main) ✗ yarn why semver
└─ normalize-package-data@npm:2.5.0
   └─ semver@npm:5.7.2 (via npm:2 || 3 || 4 || 5)
➜  project git:(main) ✗ yarn why normalize-package-data
├─ meow@npm:6.1.1
│  └─ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)
│
└─ read-pkg@npm:5.2.0
   └─ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)

In the new bundled deps version (12.1.1):

➜  project git:(main) ✗ yarn why normalize-package-data
└─ read-pkg@npm:5.2.0
   └─ normalize-package-data@npm:2.5.0 (via npm:^2.5.0)

No report, although normalize-package-data it's being used by meow

Also, how does security issues are auto checked in this repo?

@sindresorhus
Copy link
Owner

not be reported anymore by bots

I would argue that's an upside. 99% of all npm audits are not actually vulnerabilities (the semver "vulnerability" is a good example of one) or not applicative to meow. So this reduces the noise.

https://overreacted.io/npm-audit-broken-by-design/

@Grohden
Copy link

Grohden commented Nov 9, 2023

@sindresorhus I agree that audit sucks. Big red "critical" text scares security company monke

image

But as Dan says "Inlining dependencies kind of goes against the whole point of npm", and I feel like we kinda lose any analysis (you might not only use it for security) tools using that approach:

  • I can't know for sure this dep is in my codebase (if its a real issue)
  • I can't report it in bundle analysis (it's gonna report meow as a bigger dep)
  • I can't use resolutions field to force dependency version (this is somehow a win.. cuz its a nightmare to remember potential issues with the resolutions)
  • More potential to duplicate code (ties back to what Dan said)

But in the end it's your decision & opinion as a lib owner, as long as you're aware I think its ok

@sindresorhus
Copy link
Owner

I generally agree that inlining is bad, but in this specific case, the upsides outweight the downsides.

@fregante
Copy link

fregante commented Dec 22, 2023

I'm on the fence with this change… but also I think it would be great to see it on ora, which seems to be super small but in reality:

At the end of the day bundling is great for this sort of dependencies, but it does force you to release more often than you normally would. This happens for example when a new node version is out and some old issue now needs to be fixed here as well (with a simple sub-dependency patch update)

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.

Bundle dependencies
5 participants