-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Conversation
|
@@ -29,7 29,7 @@ import ThrowableDiagnostic, { | |||
} from '@parcel/diagnostic'; | |||
import json5 from 'json5'; | |||
|
|||
import {makeRe} from 'micromatch'; | |||
import {globToRegex} from '@parcel/utils'; |
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.
Centralized all uses of micromatch into the @parcel/utils
package so we only get one copy
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 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?
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.
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 |
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.
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') { |
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.
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? |
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.
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. |
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.
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...
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.
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', |
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'm surprised that invalidateOnFileCreate
can be used to watch for folder creations...
Thanks for this @devongovett, do you have plans to cut a release including these changes soon? |
@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? |
@devongovett I tried upgrading my simple project to the nightlies, and the generated output is the same, so good enough for me I guess? ;)
I reached out to know whether I should wait for an upcoming release, or commit these updates. |
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:@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.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.@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.