tmeasday / storybook-skeleton

3 stars 1 forks source link

Lazy require contexts working with lazy compilation #19

Closed tmeasday closed 3 years ago

tmeasday commented 3 years ago

Currently when I run:

PROJECT=template IMPORT=lazy-require-context COMPILE_LAZILY=1 yarn start-wds

It compiles all bundles when I visit the first story (taking around 600-700ms). When I browse to other stories although I receive more bundles I do not see webpack doing any extra work.

Contrast to

PROJECT=template IMPORT=static COMPILE_LAZILY=1 yarn start-wds

(The "static" import style is a predetermined set of static (lazy) import()s).

The compilation time of each CSF file is <200ms and it does work each time I visit a new story.

tmeasday commented 3 years ago

This is an issue in webpack with fix: https://github.com/webpack/webpack/pull/13478.

However I tried to update webpack in this branch: https://github.com/tmeasday/storybook-skeleton/tree/lazy-rc-lazy-compilation and it didn't help.

bebraw commented 3 years ago

Here's the test case from the PR to webpack: https://github.com/webpack/webpack/pull/13478/files#diff-37fd659e90e3e04fe80b7016edf6187ccaf1bf6e4d528c616dd40be494f58c07 .

Can you see any difference to the way we're using it? I wonder if we're missing a case still. In the PR, particularly

import(`./modules/${key}`)

kind of lookups are supported with lazy compilation.

tmeasday commented 3 years ago

@bebraw can confirm it works with

PROJECT=template IMPORT=dynamic COMPILE_LAZILY=1 yarn start-wds

The dynamic import style looks like:

https://github.com/tmeasday/storybook-skeleton/blob/33280053fbe5a4f6f9efbdfbfb4de7e59fb51b55/src/template-entry-import-dynamic.js#L5-L5


For storybook I think we probably need the require.context style to be general enough to match complex globs.

bebraw commented 3 years ago

For storybook I think we probably need the require.context style to be general enough to match complex globs.

Can you give an example of a complex glob that we cannot model with an import()?

I guess the implication is that we should fix require.context in webpack side if there's a case we cannot tackle with import().

sokra commented 3 years ago

It would only work with require.context in async mode, and we would need to add that to supported dependencies in webpack too.

tmeasday commented 3 years ago

It would only work with require.context in async mode, and we would need to add that to supported dependencies in webpack too.

Yep, that's what the IMPORT=lazy-require-context does, it pulls a import style like so:

https://github.com/tmeasday/storybook-skeleton/blob/574e531baf542ef09e313bf4cf32dbfc35c2c3bc/src/template-entry-import-lazy-require-context.js#L1-L6

Can you give an example of a complex glob that we cannot model with an import()?

The typical example is './components/**/*.stories.*' which I believe maps into a require context like:

require.context('./components/', true, /.*.stories\..*$/, 'lazy');

I'm not seeing a way to do import('${var}') with a recursive mode, but I could be missing something!

Either way it probably makes sense to support lazy require contexts too ;)

bebraw commented 3 years ago

I think I would try to model require.context('./components/', true, /.*.stories\..*$/, 'lazy'); with an import() like this:

import(`./components/${componentPath}`).then(...)

That componentPath can then be stories/demo.jsx and so on.

To set this into a lazy mode, you should add a comment as described at https://webpack.js.org/api/module-methods/#magic-comments (/* webpackMode: "lazy" */). There are also other options you might find handy. We can force prefetching/preloading etc. though likely we want to go through the mechanism I added (i.e. trigger import on mouseover or do something automated at the background).

To verify the behavior, this would have to be tested and I believe @sokra can correct me if I'm wrong.

tmeasday commented 3 years ago

Verified that the import(...) approach works: https://github.com/tmeasday/storybook-skeleton/pull/26#issuecomment-865910459

I guess webpack should still support require.context() and lazy compilation, but in SB6.4 I think we will switch to import() so this isn't an issue for us I would guess.