Add new package @wordpress/lazy-import for lazily installed packages
#22684
Conversation
aduth
added
[Type] Performance
aduth
requested review from
ajitbohra,
chrisvanpatten,
gziolo,
nerrad and
ntwb
as code owners
|
Size Change: 0 B Total Size: 1.12 MB ℹ️ View Unchanged
|
aduth
changed the title
@wordpress/lazy-import for lazily installed packages@wordpress/lazy-import for lazily installed packages
There was a problem hiding this comment.
I reviewed code and it looks great so far. I’m impressed how easy to follow it is taking into account all the inherent complexity. Major props for coming up with the idea of using npm aliases. Does it mean it also deduplicates dependencies? It should 😃
I will test benchmarks later. I think that the lock file needs verification. I expect it to cause some dependencies installed locally in the new package.
| ); | ||
| } | ||
|
|
||
| const localModule = getLocalModuleName( arg ); |
There was a problem hiding this comment.
If I follow it correctly, there is a tiny probability that you could install the same version of the package several times using different aliases when you use the following syntaxes:
my-packagemy-package@1.0.0my-package@^1.0.0my-package@~1.0.0.
and they all resolve to the same version.
I don't see it as a blocker for v1, but I was curious if you share my concern.
There was a problem hiding this comment.
If I follow it correctly, there is a tiny probability that you could install the same version of the package several times using different aliases when you use the following syntaxes:
Yeah, it's a good point. Earlier in my iterations, I had wondered if there might be a way to avoid the hashing altogether by naming the folder based on whichever version the specifier resolves to. It would be nice, but the problem is that it's not clear up-front (at the time of calling npm install) what that version would be, except by triggering an extra network request to fetch the version ahead of the install.
There's certainly some prior art from bin/check-latest-npm.js we could use to go this route, and it may actually be an advisable path.
One of the other things I wasn't quite as fond of is the fact that you can't tell based on the folder even what the name of the package is, let alone the version. It's only necessary because I wasn't certain if all possible characters in this argument (version specifier) are valid for folder names. But if we can know the package name and specific SemVer version, we could construct folders like:
node_modules/@wordpress/lazy-import.my-package.1.0.0/package.json
There was a problem hiding this comment.
I don't love the current implementation, but I'm having a hard time thinking if there's a good alternative.
I considered:
- Fetch from the NPM registry beforehand? Downsides are: Extra (redundant) network request, not clear how to fetch from NPM based on a version specifier (vs. a specific version like in Framework: Add package-lock precommit check for latest NPM #21306).
- Install and then rename the directory once complete? Downsides are: There's no easy way to check for the local temporary install to avoid
npm installon subsequent runs, since the specific desired version can't be known. It might technically be possible to find all locally-installed versions and run them against the version specifier usingsemverpackage.
There was a problem hiding this comment.
I agree that using an additional network request is concerning here. Well, if you use consistently one way to lazy import it won’t be an issue. I have just realized that the same issue happens when using Lerna’s monorepo setup and you pick different versions 😅Npm might deduplicate those dependencies but it often installs them multiple times...
|
I removed $ rm -rf node_modules
$ npm install
$ node packages/is-shallow-equal/benchmark/index.js
(node:88834) UnhandledPromiseRejectionWarning: Error: Cannot find module '@wordpress/lazy-import.5ab856344c555e50779f56b4cd1f2e02'
Require stack:
- /Users/gziolo/Projects/gutenberg/packages/lazy-import/lib/index.js
- /Users/gziolo/Projects/gutenberg/packages/is-shallow-equal/benchmark/index.js
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
at Function.Module._load (internal/modules/cjs/loader.js:864:27)
at Module.require (internal/modules/cjs/loader.js:1044:19)
at require (internal/modules/cjs/helpers.js:77:18)
at lazyImport (/Users/gziolo/Projects/gutenberg/packages/lazy-import/lib/index.js:111:9)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at async Promise.all (index 1)
(node:88834) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:88834) [DEP0018] DeprecationWarning: Unhandled promise rejectionsIt works properly on the second try. I'm sure we discussed a similar issue in the context of
|
Interesting that it had worked for me, then. But as you mention, it's something where there's most certainly going to be some issue with the |
|
I rebased the branch, since the recent Prettier upgrade did have an effect on how the code here was formatted (specifically the function chaining in the |
I added a cached deletion in 7b63dbc . Can you try it again? |
|
I added 86611ce that cleans up
The issue still applies. I don't think you can't use |
|
Hmm, there is this issue reported on both Travis and when I locally run: $ npx wp-scripts check-licenses --dev
WARNING Unable to locate path for missing peer dep vue@^2.6.10.
WARNING Unable to locate path for missing peer dep puppeteer@^1.10.0 || ^2.0.0.
ERROR Unable to locate path for undefined@undefined.
WARNING Unable to locate path for missing peer dep enzyme@^2.7.1. % |
Can you explain how you made these changes? I'm a bit confused by the removal of |
I'm surprised as well :) I temporarily added |
There was a problem hiding this comment.
I spent tons of time trying to find out why one of the packages fails to work on the first run and errors the benchmark script listed for testing purposes. I’ll continue investigation but I wouldn’t call it a blocker if you can’t replicate it. It works on subsequent tries or when I remove it from the file
| lazyImport( 'shallow-equals@^1.0.0' ), | ||
| new Promise( async ( resolve ) => { | ||
| try { | ||
| await lazyImport( 'fbjs@^1.0.0' ); |
There was a problem hiding this comment.
For future: it would be great to support natively packages that don’t have an entry point or custom path from the package.
There was a problem hiding this comment.
For future: it would be great to support natively packages that don’t have an entry point or custom path from the package.
Yeah, the thought had come to mind, especially since the previous implementation was able to easily import from a nested path, whether it's something that could be supported in this implementation as well.
I'll give it a quick check to see if it might be straight-forward to add support. Otherwise, I can create an issue to follow up.
There was a problem hiding this comment.
I'll give it a quick check to see if it might be straight-forward to add support. Otherwise, I can create an issue to follow up.
npm-package-arg doesn't seem to handle the path very well on its own. It might be something where we can either implement the directory parsing or the entire module / version spec parsing as something custom, but I'd suggest it be done separately.
There was a problem hiding this comment.
Thanks for opening a follow up issue 💯
packages/lazy-import/lib/index.js
Outdated
| } | ||
|
|
||
| // Delete cache from prior `require` attempt. | ||
| delete require.cache[ require.resolve( localModule ) ]; |
There was a problem hiding this comment.
In my testing, this line doesn't work when something goes wrong. We can remove it. If the package is properly installed then require in the next line works regardless.
There was a problem hiding this comment.
I was puzzled about this, since it seems like given the intention of require.cache to bypass a second lookup would conflict with the intention of the following line to try the import once more.
But I guess it depends when a module is cached. Notably, the use-case here can assume that the previous attempt would have been a failure.
Briefly testing this in the terminal, it does appear that a module will not be cached if it couldn't be resolved:
⇒ node -e "try { require( './abc.js' ); } catch {} console.log( require.cache );"
[Object: null prototype] {}
So, as you mention, I suppose it should be safe to remove.
|
It feels so great to see it merged, let’s see how it works in action 🎉 |
This pull request seeks to introduce a new package
@wordpress/lazy-import, which can be used to dynamically install modules on-demand. The proposed usefulness here is in cases where modules are used in ad-hoc scripts (see included changes for benchmarking dependencies) or in cases where the dependency is only used in particular logic paths which may not always be encountered, or at least not frequently enough to warrant the up-front cost of the install.@wordpress/scriptsis a good candidate for this, where this package is very similar to what was done in #20215, but extrapolated for any dependency.As mentioned, the changes include removal of top-level dependencies for various shallow-equals implementations which are used only for the (rarely-run) benchmarking script bundled with
@wordpress/is-shallow-equal.Future potential usage could include lazily importing individual dependencies of
@wordpress/scripton a per-command basis (e.g. install Jest only if and when runningtest-unit, Prettier when runningformat-js, ESLint when runninglint-js, etc).Implementation Notes:
The lazy-require package served as some inspiration for this one. However, aside from the fact that there are some issues with it (bevry-archive/lazy-require#48), it does not support an option to specify a version of the package to use. This can be problematic, since it could potentially mean that a later major version update of a package could break expected usage.
With this implementation, the usage is such that you can call it with an argument string as would be passed to
npm install, including an optional version specifier:This is partly inspired by RunKit, which works this way when using
require:https://runkit.com/embed/qxtd3i7j9org
Internally, it uses the same modules used by NPM to parse and validate the version specifier, so it should act exactly as one should expect as if the string is passed to
npm install.Normally, it is not possible to install multiple versions of the same package to a local project. To work around this limitation, the implementation (ab)uses npm aliases, where a unique name is generated using in the input argument.
For illustration purposes, given an input
shallow-equal@^1.2.1, it behaves like:...where
5ab856344c555e50779f56b4cd1f2e02is an md5 hash of the inputshallow-equal@^1.2.1.Testing Instructions:
Verify that the benchmarking script still runs without issue, even after clearing
node_modulesand installing with the benchmark dependencies removed:The first run may take some additional time since the dependencies must be installed on-demand. Subsequent runs should start faster (though still delayed, since benchmarking itself takes some time to run).