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

Reduce the number of npm dependencies needed by parcel #7576

Merged
merged 14 commits into from
Jan 27, 2022
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Jan 19, 2022

Previously, installing the parcel package downloaded 493 dependencies from npm. This was made better from the over 600 deps we used to have in #7564, but is still pretty ridiculous. The aim of this PR is to reduce this further. This is accomplished in a few ways:

  • Pre-bundling some dependencies with Parcel itself rather than loading them from npm. This is done mainly for small utility dependencies, not for things that applications would normally need to control the versions of. Specifically, for the following packages: @parcel/fs, @parcel/package-manager, @parcel/utils, @parcel/codeframe, @parcel/reporter-cli, and @parcel/reporter-dev-server. This reduces dependencies to 334. It also has the benefit that any random security warnings produced by npm will only affect the Parcel team and not downstream users. Most of the security warnings we receive are not actually exploitable in Parcel anyway, so this really is just about reducing annoyance for everyone. Both Next.js and Vite have taken a similar approach.
  • Auto installing node builtin polyfills on demand (e.g. buffer, stream, etc.). The @parcel/node-libs-browser package pulled in over 100 dependencies, which were pretty rarely used these days. Now you'll see a warning when one is encountered, and it'll be auto installed into your project. In combination with the above, this reduces dependencies to 226.
  • Removing built-in Babel and PostCSS dependencies, and installing them into projects on demand, only when actually used. This includes builtin flow and legacy css modules support. In combination with the above, this reduces dependencies to 183.
  • Removing CSS nano once we switch entirely to @parcel/css will reduce this further to 150.

There are other potential optimizations we can do in the future, but this already helps a lot, reducing dependencies by ~70%. It should result in faster install times, and less dependencies to audit for everyone using Parcel.

@height
Copy link

height bot commented Jan 19, 2022

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@@ -29,7 29,7 @@ import ThrowableDiagnostic, {
} from '@parcel/diagnostic';
import json5 from 'json5';

import {makeRe} from 'micromatch';
import {globToRegex} from '@parcel/utils';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Centralized all uses of micromatch into the @parcel/utils package so we only get one copy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall that there was some breaking change in micromatch that thwarted a previous attempt to consolidate it (see this comment from @mischnic on a PR I forgot about 😨 ). Not sure if it's still relevant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one occurence was fixed in #6958

if (process.env.PARCEL_BUILD_ENV !== 'production') {
if (
process.env.PARCEL_BUILD_ENV !== 'production' ||
process.env.PARCEL_SELF_BUILD
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new environment variable is needed so we can replace PARCEL_BUILD_ENV while parcel is building itself...

production: {
module.exports = api => {
let name = api.caller(caller => caller && caller.name);
if (name === 'parcel') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A different Babel config is used when Parcel is building itself since we don't want to compile modules etc.

@@ -79,10 77,22 @@ export default (new Transformer({
let cssModules: ?{|[string]: string|} = null;
if (config.hydrated.modules) {
asset.meta.cssModulesCompiled = true;

// TODO: should this be resolved from the project root?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually a question for all dev deps. What if there were a CSS module in node_modules? we would never want to install dev deps there...

@@ -602,7 653,16 @@ export default class NodeResolver {
if (isExplicitNode) {
filename = filename.substr(5);
}
return {filePath: builtins[filename] || empty};

// By default, exclude node builtins from libraries unless explicitly opted in.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good idea? Alternatively we could have some separate config for it, but it seems like when building libraries you'd usually want to exclude these...

Copy link
Contributor

@Shinyaigeek Shinyaigeek Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is ideal to resolve node builtins polyfill with the bundler of the library user, I feel we do not want to include node builtins if a bundling target is a library working in the browser. On the other hand, I think some library author wants to host their library on esm.sh etc. and use it as pure ESModules from the browser. In that case, the bundle of the library should include node builtins.

So, I think it is good to make it the default behavior, but I think also that an emergency hatch should be prepared as a config or flag for such use cases. (it would be simple to judge that with whether or not the output is esmodule, but in reality, it shouldn't be so difficult ….)

I'm sorry if I overlooked the context and said something strange

@@ -297,8 297,8 @@ describe('resolver', function () {
aboveFilePath: path.join(rootDir, 'foo.js'),
},
{
fileName: 'package.json',
aboveFilePath: require.resolve('browserify-zlib'),
fileName: 'node_modules/browserify-zlib',
Copy link
Member

@mischnic mischnic Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that invalidateOnFileCreate can be used to watch for folder creations...

@devongovett devongovett merged commit 4cbda87 into v2 Jan 27, 2022
@devongovett devongovett deleted the reduce-deps branch January 27, 2022 15:47
devongovett added a commit that referenced this pull request Jan 31, 2022
@xvello
Copy link

xvello commented Feb 5, 2022

Thanks for this @devongovett, do you have plans to cut a release including these changes soon?

@devongovett
Copy link
Member Author

@xvello i hope so. We just want to do some testing to ensure nothing is broken. If you want, you could try out the nightly release and report back if it works for you?

@xvello
Copy link

xvello commented Feb 5, 2022

@devongovett I tried upgrading my simple project to the nightlies, and the generated output is the same, so good enough for me I guess? ;)

npm install --dev [email protected]
npm install -dev @parcel/[email protected] 90050e4e

I reached out to know whether I should wait for an upcoming release, or commit these updates.

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.

5 participants