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

Allow replacing of stories with a warning (rather than an error) #4061

Merged
merged 3 commits into from
Sep 8, 2018
Merged

Allow replacing of stories with a warning (rather than an error) #4061

merged 3 commits into from
Sep 8, 2018

Conversation

mikeduminy
Copy link
Contributor

Issue: #4056

What I did

Replaced the error thrown when adding an already existing story with a console.warn. Created a test that covers this case. Note: console.warn is mocked as the jest config throws an error when this is called.

allow replacing of stories
9d23bcb 
fixes storybookjs#4056

remove try catch in test
@mikeduminy mikeduminy mentioned this pull request Aug 23, 2018
Closed
@mikeduminy
Copy link
Contributor Author

I originally made the changes on a release branch so I'm just running through the tests on master to make sure nothing has broken.

@mikeduminy
Copy link
Contributor Author

mikeduminy commented Aug 23, 2018 •
edited
Loading

I can fix the tests for master, but that will break the tests for 3.4 when and if the commit is cherry picked.

EDIT: I'll try fix them for both.

igor-dv
igor-dv reviewed Aug 23, 2018
View reviewed changes
lib/core/src/client/preview/client_api.js
@@ -95,7 +95,7 @@ export default class ClientApi {
}

if (this._storyStore.hasStory(kind, storyName)) {
throw new Error(`Story of "${kind}" named "${storyName}" already exists`);
logger.warn(`Story of "${kind}" named "${storyName}" already exists`);
Copy link
Member

@igor-dv igor-dv Aug 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Basically, I agree with this change, though I wonder whether we want to replace an existing story in the store with this kind
@tmeasday you've worked a lot in this area. WDYT ?

@Hypnosphi Hypnosphi added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Aug 23, 2018
@codecov
Copy link

codecov bot commented Aug 23, 2018 •
edited
Loading

Codecov Report

Merging #4061 into master will increase coverage by 48.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4061       +/-   ##
===========================================
+ Coverage   40.59%   88.63%   +48.04%     
===========================================
  Files         483        6      -477     
  Lines        5747       44     -5703     
  Branches      770        2      -768     
===========================================
- Hits         2333       39     -2294     
+ Misses       3042        4     -3038     
+ Partials      372        1      -371
Impacted Files Coverage Δ
...toryshots-core/src/frameworks/svelte/renderTree.js
addons/backgrounds/mithril.js
app/riot/src/client/preview/render-riot.js
addons/knobs/src/components/types/Select.js
...yshots/storyshots-core/src/frameworks/rn/loader.js
app/react/src/server/wrapDefaultBabelConfig.js
lib/ui/src/modules/ui/configs/handle_routing.js
...ents/stories_panel/stories_tree/tree_decorators.js
...-native/src/preview/components/OnDeviceUI/style.js
lib/core/src/server/config.js
... and 466 more

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 7b2b4e0...836622f. Read the comment docs.

@igor-dv
Copy link
Member

igor-dv commented Aug 27, 2018

@storybooks/team, does anyone want to give a feedback?

wuweiweiwu
wuweiweiwu reviewed Aug 28, 2018
View reviewed changes
Copy link
Member

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

LGTM. Nice tests!

@mikeduminy
Copy link
Contributor Author

Thanks for the approval. If there's anything I can do to get this into the v3 stream please let me know.

@ndelangen
Copy link
Member

I think we need to merge in master, I can do it today.

@Hypnosphi
Copy link
Member

@michaelduminy I will cherry-pick it to release/3.4 branch right before the next patch release, you don't need to do anything

@Hypnosphi Hypnosphi self-assigned this Sep 8, 2018
@Hypnosphi Hypnosphi merged commit b96f1d3 into storybookjs:master Sep 8, 2018
@shilman shilman changed the title Allow replacing of stories (again) Allow replacing of stories with a warning (instead of throwing an error) Sep 19, 2018
@shilman shilman changed the title Allow replacing of stories with a warning (instead of throwing an error) Allow replacing of stories with a warning (rather than an error) Sep 19, 2018
Hypnosphi added a commit that referenced this pull request Sep 19, 2018
@Hypnosphi
Merge pull request #4061 from michaelduminy/master
01731df 
Allow replacing of stories (again)
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wuweiweiwu wuweiweiwu wuweiweiwu left review comments

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

@Hypnosphi Hypnosphi Hypnosphi approved these changes

@ndelangen ndelangen ndelangen approved these changes

@alterx alterx Awaiting requested review from alterx alterx is a code owner

@tmeasday tmeasday Awaiting requested review from tmeasday tmeasday is a code owner

Assignees

@Hypnosphi Hypnosphi

Labels
bug 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 react-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mikeduminy @igor-dv @ndelangen @Hypnosphi @wuweiweiwu @shilman