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

Restore v4 webpack extend mode behavior AND deprecate it #6104

Merged
merged 8 commits into from
Mar 16, 2019
Merged

Restore v4 webpack extend mode behavior AND deprecate it #6104

merged 8 commits into from
Mar 16, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Mar 15, 2019 •
edited
Loading

Issue: #6081 #5941 #6083 #5708 #6111

What I did

How to test

  1. Update official-storybook's webpack.config.js to export {}
  2. Observe deprecation message in console

@shilman shilman added configuration babel / webpack maintenance User-facing maintenance tasks labels Mar 15, 2019
@shilman shilman added this to the v5.1.0 milestone Mar 15, 2019
@shilman shilman mentioned this pull request Mar 15, 2019
Closed
4 tasks
tmeasday
tmeasday requested changes Mar 15, 2019
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM although I can't speak to the exact webpack config :)

MIGRATION.md Outdated
Please see the [current custom webpack documentation](https://github.com/storybooks/storybook/blob/next/docs/src/pages/configurations/custom-webpack-config/index.md) for more information on custom webpack config.
### Deprecated extend mode

Exporting an object from your custom webpack config puts storybook in "extend mode". This is still supported in 5.x but we've deprecated this and encourage users to use full-control mode instead.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a code snippet of how to achieve the exact same behaviour of extend mode with full control mode?

@codecov
Copy link

codecov bot commented Mar 15, 2019 •
edited
Loading

Codecov Report

Merging #6104 into next will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #6104      +/-   ##
=========================================
- Coverage   37.1%   37.09%   -0.02%     
=========================================
  Files        648      649       +1     
  Lines       9524     9528       +4     
  Branches    1381     1352      -29     
=========================================
  Hits        3534     3534              
- Misses      5411     5415       +4     
  Partials     579      579
Impacted Files Coverage Δ
lib/core/merge-webpack-config.js 0% <0%> (ø)
...b/core/src/server/preview/custom-webpack-preset.js 0% <0%> (ø) ⬆️

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 fa04c38...d30b43d. Read the comment docs.

@shilman shilman changed the title Deprecate webpack extend mode and update docs Restore v4 webpack extend mode behavior AND deprecate it Mar 15, 2019
@shilman shilman added bug and removed maintenance User-facing maintenance tasks labels Mar 15, 2019
@codecov-io
Copy link

codecov-io commented Mar 15, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #6104 into next will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #6104      +/-   ##
=========================================
- Coverage   37.1%   37.09%   -0.02%     
=========================================
  Files        648      648              
  Lines       9524     9528       +4     
  Branches    1381     1381              
=========================================
  Hits        3534     3534              
- Misses      5411     5415       +4     
  Partials     579      579
Impacted Files Coverage Δ
addons/a11y/src/components/A11YPanel.tsx 94.44% <ø> (ø) ⬆️
...b/core/src/server/preview/custom-webpack-preset.js 0% <0%> (ø) ⬆️

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 fa04c38...3aa6cfc. Read the comment docs.

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 15, 2019
@shilman shilman modified the milestones: v5.1.0, 5.0.x Mar 15, 2019
igor-dv
igor-dv reviewed Mar 15, 2019
View reviewed changes
lib/core/src/server/preview/custom-webpack-preset.js
@@ -28,5 +30,15 @@ export async function webpack(config, options) {
}

logger.info('=> Loading custom webpack config (extending mode).');
return mergeConfigs(finalDefaultConfig, customConfig);

// Restore 4.x behavior, but deprecate this mode of extending webpack
Copy link
Member

Choose a reason for hiding this comment

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

webpackFinal will be called twice again =(

since we are not passing the baseConfig to the full control mode function, we can just move the call of the finalWebpack preset from line 16 into two first conditions (lines 24 and 29)

lib/core/merge-webpack-config.js Outdated Show resolved Hide resolved
@shilman shilman merged commit 55547f3 into next Mar 16, 2019
@shilman shilman deleted the 6081-deprecate-extend-mode branch March 16, 2019 06:19
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2019 •
edited
Loading

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 3aa6cfc

@shilman shilman removed the bug label Mar 16, 2019
This was referenced Mar 16, 2019
Closed
Closed
Closed
Closed
shilman added a commit that referenced this pull request Mar 16, 2019
@shilman
Merge pull request #6104
9cf2866
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 16, 2019
@shilman shilman mentioned this pull request Apr 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-dv igor-dv igor-dv left review comments

@tmeasday tmeasday tmeasday requested changes

@ndelangen ndelangen ndelangen approved these changes

@alterx alterx Awaiting requested review from alterx

@Hypnosphi Hypnosphi Awaiting requested review from Hypnosphi

@alexandrebodin alexandrebodin Awaiting requested review from alexandrebodin

@jbovenschen jbovenschen Awaiting requested review from jbovenschen

@naipath naipath Awaiting requested review from naipath

Assignees
No one assigned
Labels
BREAKING CHANGE configuration babel / webpack patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Milestone
5.0.x
Development

Successfully merging this pull request may close these issues.
5 participants
@shilman @codecov-io @tmeasday @ndelangen @igor-dv