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

Lazy Import: Allow local paths as an option when importing #23751

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 7, 2020

Description

A try to unblock #23712 where we want to add support for external templates installed from npm in @wordpress/create-block.

We miss a way to lazy import a local path from a package. It was only possible to import the script listed as main by the package.

This PR adds a new option localPath that allows importing files other than those listed as main for the package.

How has this been tested?

$ rm -rf node_modules
$ npm install
$ node packages/is-shallow-equal/benchmark/index.js`

Should run all the benchmarks on the @wordpress/is-shallow-equal package.

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@gziolo gziolo added the [Type] New API New API to be used by plugin developers or package users. label Jul 7, 2020
@gziolo gziolo requested a review from aduth July 7, 2020 13:44
@gziolo gziolo self-assigned this Jul 7, 2020
@gziolo gziolo added the [Package] Lazy import /packages/lazy-import label Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020

Size Change: 0 B

Total Size: 1.21 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 9.02 kB 0 B
build/block-library/editor.css 9.02 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.84 kB 0 B
build/block-library/style.css 7.85 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.5 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/index.js 22.4 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo gziolo changed the title Lazy Import: Allow local paths as an opton when importing Lazy Import: Allow local paths as an option when importing Jul 7, 2020
@aduth
Copy link
Member

aduth commented Jul 7, 2020

This would be really useful. Do you think it might be possible for the usage to be more like how one might expect to use require or import?

In other words, instead of...

lazyImport( 'fbjs@^1.0.0', {
	localPath: './lib/shallowEqual',
} );

I would expect:

lazyImport( 'fbjs/lib/shallowEqual@^1.0.0' );

It might be slightly more challenging to implement, since it means we probably can't take advantage of the existing usage of npm-package-arg to manage the string parsing, and instead do more of it manually.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Jul 9, 2020
@gziolo
Copy link
Member Author

gziolo commented Oct 27, 2020

It turns out that the usage of npmPackageArg is quite flexible as it allows to install npm packages using a variety of ways:

  • arg - a string that you might pass to npm install, like:
    foo@1.2, @bar/foo@1.2, foo@user/foo, http://x.com/foo.tgz,
    git+https://github.com/user/foo, bitbucket:user/foo, foo.tar.gz,
    ../foo/bar/ or bar. If the arg you provide doesn't have a specifier
    part, eg foo then the specifier will default to latest.

If we would decide to reimplement it ourselves we would lose some of those options or we would have to find a common approach on how to deal with local paths. Now the question is whether we want to introduce a special case:

lazyImport( 'fbjs/lib/shallowEqual@^1.0.0' );

or we are fine with a more verbose API but also one that would work with all representations of arg that npmPackageArg is able to parse.

@gziolo gziolo requested a review from nerrad October 28, 2020 08:55
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Oct 28, 2020
@mkaz
Copy link
Member

mkaz commented Nov 3, 2020

Error while building

I think you need to add @wordpress/lazy-import to packages/is-shallow-equal/package.json

>> node packages/is-shallow-equal/benchmark/index.js
(node:10474) UnhandledPromiseRejectionWarning: Error: Cannot find module '@wordpress/lazy-import.5ab856344c555e50779f56b4cd1f2e02'
Require stack:
- /home/mkaz/src/wp/gutenberg/packages/lazy-import/lib/index.js
- /home/mkaz/src/wp/gutenberg/packages/is-shallow-equal/benchmark/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:831:15)
    at Function.Module._load (internal/modules/cjs/loader.js:687:27)
    at Module.require (internal/modules/cjs/loader.js:903:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at lazyImport (/home/mkaz/src/wp/gutenberg/packages/lazy-import/lib/index.js:138:9)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Promise.all (index 1)

@gziolo
Copy link
Member Author

gziolo commented Nov 3, 2020

Error while building

I think you need to add @wordpress/lazy-import to packages/is-shallow-equal/package.json

I think you run into the same strange issue with one of the dependencies from the benchmark, it should work on every subsequent install. I was able to reproduce it every time but it always worked for @aduth 🤷‍♂️ Upon investigation, it was always only one of the packages used in the benchmark.

See: #22684 (comment)

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

Ok, it works on second run and looks good. 👍

@gziolo gziolo merged commit ad62189 into master Nov 5, 2020
@gziolo gziolo deleted the update/lazy-import-path branch November 5, 2020 16:33
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Lazy import /packages/lazy-import [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants