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

Builder-Webpack5: Use native features instead of plugins #14281

Merged
merged 5 commits into from
May 13, 2021
Merged

Builder-Webpack5: Use native features instead of plugins #14281

merged 5 commits into from
May 13, 2021

Conversation

7rulnik
Copy link
Contributor

@7rulnik 7rulnik commented Mar 20, 2021

What I did

How to test

Just run some examples with assets.


Blocked by #14264

@shilman shilman added builder-webpack5 maintenance User-facing maintenance tasks labels Mar 20, 2021
@shilman
Copy link
Member

shilman commented Mar 20, 2021

@7rulnik Awesome, tremendous work! 😱😱😱 Given that we are late in the release cycle and these are fairly core changes, I'd like to merge this into 6.3-alpha. Are there any important reasons why we should merge this now (and possibly delay the 6.2 release as a result?). For example, are these breaking changes in builder-webpack5? Thanks for your help and your patience!

@7rulnik
Copy link
Contributor Author

7rulnik commented Mar 20, 2021

@shilman no need to rush, but there may be only one reason — not sure that the current config for webpack 5 works with yarn PnP. Have you tested this?

@shilman
Copy link
Member

shilman commented Mar 20, 2021

@7rulnik No, I certainly haven't. I'll discuss with @gaetanmaisse who is our local PnP guru. We're calling webpack5 support experimental in 6.2, so I think we can ship without PnP support if it turns out to be broken. Another big limitation is that react-docgen-typescript-plugin needs to be updated for webpack5 support. I'll document these known limitations and then hopefully we can get out fixes early in 6.3.

@7rulnik 7rulnik marked this pull request as draft March 20, 2021 19:59
@shilman
Copy link
Member

shilman commented Mar 22, 2021

Merged the blocking PR and will release today FYI

@7rulnik 7rulnik marked this pull request as ready for review March 23, 2021 08:54
@7rulnik
Copy link
Contributor Author

7rulnik commented Mar 23, 2021

@shilman I'm trying to run an example and update test but got error from yarn bootstrap

$ node ../../scripts/prepare.js
Successfully compiled 7 files with Babel (2038ms).

Successfully compiled 7 files with Babel (1133ms).

TSFILE: /Users/v7rulnik/projects/aviasales/oss/storybook/lib/postinstall/dist/ts3.9/frameworks.d.ts
TSFILE: /Users/v7rulnik/projects/aviasales/oss/storybook/lib/postinstall/dist/ts3.9/presets.d.ts
TSFILE: /Users/v7rulnik/projects/aviasales/oss/storybook/lib/postinstall/dist/ts3.9/index.d.ts
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

lerna ERR! yarn run prepare stderr:
ERR! FAILED (ts) :
ERR! FAILED to compile ts: @storybook/postinstall@6.2.0-rc.8
error Command failed with exit code 1.

lerna ERR! yarn run prepare exited 1 in '@storybook/postinstall'
lerna WARN complete Waiting for 5 child processes to exit. CTRL-C to exit immediately.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

```

@shilman
Copy link
Member

shilman commented Mar 23, 2021

@7rulnik what's your setup? and what's the actual error? pretty sure this is working for me

MMBP15:storybook shilman$ npx sb info

Environment Info:

  System:
    OS: macOS Mojave 10.14.6
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 12.21.0 - ~/n/bin/node
    Yarn: 1.22.10 - ~/n/bin/yarn
    npm: 6.14.11 - ~/n/bin/npm
  Browsers:
    Chrome: 89.0.4389.90
    Firefox: 85.0.2
    Safari: 14.0.3

@7rulnik
Copy link
Contributor Author

7rulnik commented Mar 23, 2021

@shilman it's weird because I copy-pasted all info from the console and there is not an exact error. I decided to debug a bit the prepare script, but after reboot, all works fine. By the way, I'm on M1 MacBook

Environment Info:

  System:
    OS: macOS 11.2
    CPU: (8) x64 Apple M1
  Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    Yarn: 1.22.10 - /opt/homebrew/bin/yarn
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm
  Browsers:
    Chrome: 89.0.4389.90
    Firefox: 84.0.2
    Safari: 14.0.3
  npmPackages:
    @storybook/eslint-config-storybook: ^2.4.0 => 2.4.0
    @storybook/linter-config: ^2.5.0 => 2.5.0
    @storybook/semver: ^7.3.2 => 7.3.2

But as I can see we have some problems with PnP case on CI?

@shilman
Copy link
Member

shilman commented Mar 24, 2021

@7rulnik CI is broken right now due to an unrelated issue. Can you merge next into your branch and see if that fixes it?

@gaetanmaisse
Copy link
Member

Not sure that the current config for webpack 5 works with yarn PnP. Have you tested this?

I tried it (maybe before RCs go out) with @storybook/html when working on #13907 and if I remember correctly it was ok, at least in that case.

Currently, there is no E2E tests using webpack 5 I think 🤔 so I will need some manual testing. As of 6.3-alpha I will add an PnP E2E tests related to WebComponents so it could also use webpack 5 too, I need to discuss that with other WC foxes.

@gaetanmaisse
Copy link
Member

hey @7rulnik can you update this branch with next or allow SB members to push on the branch of this PR?

@shilman
Copy link
Member

shilman commented Apr 30, 2021

hey @7rulnik can you update this branch with next or allow SB members to push on the branch of this PR?

@nx-cloud
Copy link

nx-cloud bot commented Apr 30, 2021

Nx Cloud Report

CI ran the following commands for commit a030edf. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@7rulnik
Copy link
Contributor Author

7rulnik commented Apr 30, 2021

@shilman done

@shilman
Copy link
Member

shilman commented May 12, 2021

@7rulnik can you try again? we still can't push to this branch. thanks!

@7rulnik
Copy link
Contributor Author

7rulnik commented May 12, 2021

@shilman I don't have a checkbox allow edits from maintainers because it's my company org, not mine. But I sent invites to this forks to you and @gaetanmaisse

@shilman shilman changed the title Use native webpack 5 features instead of plugins Builder-Webpack5: Use native features instead of plugins May 13, 2021
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.

LGTM! Thanks @7rulnik !!!

@shilman shilman merged commit 1f97f82 into storybookjs:next May 13, 2021
@shilman shilman deleted the webpack-5-optimizations branch May 13, 2021 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-webpack5 maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants