-
Notifications
You must be signed in to change notification settings - Fork 53
fix: lazy load imports #433
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
Conversation
|
I'm getting the following error when running Hey @wraithgar, you once told me that this could happen but I have no clue why, would you mind giving some hints on what I should do in those cases? |
|
@H4ad it means we can't lazy load those modules. npm is a special unicorn in that it needs to have all modules loaded beforehand that it will need to install itself. They have to be in the require cache because we are about to remove the old npm so we can install the new npm. One of the smoke tests we have in npm itself is a check for this specifically. |
|
Would be crazy to have a list of all deps that we need to install itself to warmup the cache before doing the install global? In this way, we could lazy load everything that we want without worrying it would not be in the cache. It will probably slowdown the installation of itself for a few ms but will save some time during normal operations. |
Yeah that's a tall order, and it would be even harder to make it automatic. Sure we could manually find out but if it changes in one of the many subdependencies then all of a sudden we're broken again. |
Can't this be solved using the same principle as |
In theory yes this would work w/ the cli. We already have a |
|
Just realized this will NOT work from a centralized standpoint because we can not force subdependencies to load any unhoisted deps. So if the dependency tree looked like: graph LR;
npm-->pacote;
npm-->other-1["[email protected]"];
pacote-->other-2["[email protected]"];
We could tell npm to We'd somehow have to have a mechanism for loading all subdependencies. Consider a tree w/ ~/D/s/a $ npm ls semver
[email protected] /Users/wraithgar/Development/scratch/a
├─┬ [email protected]
│ └── [email protected]
└── [email protected]If I > Object.keys(require.cache).filter(k => k.includes('semver'))
[
'/Users/wraithgar/Development/scratch/a/node_modules/semver/semver.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/functions/valid.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/functions/parse.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/classes/semver.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/debug.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/constants.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/re.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/parse-options.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/identifiers.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/functions/clean.js'
]Notice how we have more than just the root exports to worry about. We have an even deeper problem here: require('./node_modules/normalize-package-data/node_modules/semver')
> Object.keys(require.cache).filter(k => k.includes('semver'))
[
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/index.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/re.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/constants.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/debug.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/classes/semver.js',
'/Users/wraithgar/Development/scratch/a/node_modules/normalize-package-data/node_modules/semver/internal/parse-options.js',
...
]This would mean having to know which of those deps were hoisted or not. Pretty complex tree walking just to solve this. I realize this feels discouraging, but unfortunately it's just something npm has to deal with. Every other package out there gets to use npm to install itself, and thus does not have to worry about this. |
|
One possible solution would be deduplication, but that would create friction when updating dependencies. Well, I think there might be a solution, but it might not be worth the complexity of handling something so delicate. One of my next strategies would be to try adding multiple exports for the same package, which could also make the lazy-loading task even more complex. Thanks for the detailed explanation, I will keep up trying to find other optimizations that does not involve lazy loading for npm install. |
Oh boy. Deduplication is always something we pay attention to during deps updates, and we also try to make sure the production dep is the one that gets hoisted since that's the one that gets bundled. npm/cli#8576 for example took a LOT of that. Most folks don't notice it haha. Unfortunately sometimes we do have to ship with duplicated dependencies, even if only temporarily. |
This PR basically lazy loads almost all the imports.
The image below shows how much time it spends just to
requirethis package.With this change, it went down from
52msto7ms:Further optimizations
We can reduce even more the cost to just
1msif we change thelib/index.jsto something like this:But this change a little bit the behavior of
index.jsand the exports, so it's up to you to decide if it's worthy.**Draft: In draft since I'm investing why this change broke the
npm i -g npm