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.

Sign up for GitHub

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

Jump to bottom

Webpack5: Don't import a second file while a first one is in flight #18432

Merged
merged 9 commits into from
Jun 21, 2022
Merged

Webpack5: Don't import a second file while a first one is in flight #18432

merged 9 commits into from
Jun 21, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jun 8, 2022 •
edited
Loading

Issue: Lazy compilation can fail if CSF files are imported very quickly, either by navigating or via preloading (#17964)

webpack/webpack#15541

What I did

Introduce a "pipeline" into import() when Lazy Compilation is active. The pipeline will stop us from running import() a second time while a first import() is in flight.

How to test

Turn on lazy compilation in the react-ts example:

  core: {
    channelOptions: { allowFunction: false, maxDepth: 10 },
    disableTelemetry: true,
    builder: {
      name: '@storybook/builder-webpack5',
      options: {
        lazyCompilation: true,
      },
    },
  },

NOTE

@yannbf this will not help with the test runner bug as it simply "debounces" things on the client side, but in the case of the TR, there are multiple clients, so that cannot happen. A true fix is (maybe?) to do a similar pipelining in on the lazy compilation backend in webpack.

@tmeasday tmeasday added bug patch:yes Bugfix & documentation PR that need to be picked to main branch builder-webpack5 labels Jun 8, 2022
@tmeasday tmeasday requested a review from ndelangen June 8, 2022 03:21
@nx-cloud
Copy link

nx-cloud bot commented Jun 8, 2022 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ca1a7a5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Jun 8, 2022 •
edited
Loading

Codecov Report

Merging #18432 (bdd6f61) into next (7a48cd5) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             next   #18432       /-   ##
==========================================
- Coverage   32.01%   32.00%   -0.01%     
==========================================
  Files         975      975              
  Lines       19234    19239        5     
  Branches     4033     4036        3     
==========================================
  Hits         6158     6158              
- Misses      12513    12518        5     
  Partials      563      563
Impacted Files Coverage Δ
...lder-webpack5/src/preview/iframe-webpack.config.ts 0.00% <0.00%> (ø)
lib/core-common/src/utils/to-importFn.ts 41.66% <0.00%> (-20.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a48cd5...bdd6f61. Read the comment docs.

@tmeasday tmeasday mentioned this pull request Jun 8, 2022
Open
@shilman shilman changed the title Don't import a second file while a first one is in flight Webpack5: Don't import a second file while a first one is in flight Jun 13, 2022
@tmeasday tmeasday force-pushed the tom/sb-347-sequence-importing-stories-in-parallel branch from bdd6f61 to 1f02c5c Compare June 15, 2022 05:08
@tmeasday tmeasday changed the base branch from next to future/base June 15, 2022 05:09
@codecov-commenter
Copy link

Codecov Report

Merging #18432 (1f02c5c) into future/base (a1c81d1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@               Coverage Diff               @@
##           future/base   #18432       /-   ##
===============================================
- Coverage        30.74%   30.73%   -0.01%     
===============================================
  Files              798      798              
  Lines            17086    17091        5     
  Branches          3528     3531        3     
===============================================
  Hits              5253     5253              
- Misses           11408    11413        5     
  Partials           425      425
Impacted Files Coverage Δ
...lder-webpack5/src/preview/iframe-webpack.config.ts 0.00% <0.00%> (ø)
lib/core-common/src/utils/to-importFn.ts 41.66% <0.00%> (-20.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1c81d1...1f02c5c. Read the comment docs.

@ndelangen ndelangen mentioned this pull request Jun 17, 2022
Merged
@tmeasday tmeasday removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 20, 2022
shilman
shilman requested changes Jun 20, 2022
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

CI build step is failing

@shilman shilman merged commit 203a2c0 into future/base Jun 21, 2022
@shilman shilman deleted the tom/sb-347-sequence-importing-stories-in-parallel branch June 21, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannbf yannbf yannbf left review comments

@shilman shilman shilman requested changes

@ndelangen ndelangen Awaiting requested review from ndelangen

Assignees

@ndelangen ndelangen

Labels
bug builder-webpack5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tmeasday @codecov-commenter @shilman @yannbf @ndelangen