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

Core: Add skip dispose option to ClientApi #9868

Merged
merged 7 commits into from
Feb 19, 2020
Merged

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Feb 15, 2020

Issue: storybookjs/react-native#11

What I did

Added an option to skip dispose call inside storiesOf.add() .

Relevant PR:

storybookjs/react-native#30

How to test

Added tests.

@Gongreg Gongreg requested review from tmeasday, shilman and ndelangen and removed request for tmeasday February 15, 2020 22:35
@shilman shilman changed the title added ability to skip dispose call in clientApi Core: Add skip dispose option to ClientApi Feb 16, 2020
@shilman
Copy link
Member

shilman commented Feb 16, 2020

cc @tmeasday

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.

@Gongreg I wonder if the option could be called something a bit clearer, maybe noStoryModuleHotDispose? Also perhaps we could put a comment in around the option explaining the rationale as we are sure to forget why it is there

lib/client-api/src/client_api.test.ts Outdated Show resolved Hide resolved
expect(module.hot.dispose.calls.length).toEqual(2);
});

it('should not bind dispose inside add when disableAddStoryDispose is true', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should not bind dispose inside add when disableAddStoryDispose is true', () => {
it('should not call `module.hot.dispose` inside add when disableAddStoryDispose is true', () => {

@Gongreg
Copy link
Member Author

Gongreg commented Feb 16, 2020

@Gongreg I wonder if the option could be called something a bit clearer, maybe noStoryModuleHotDispose? Also perhaps we could put a comment in around the option explaining the rationale as we are sure to forget why it is there

I am having trouble with thinking of the name. This is not fully disabling hot story dispose. This is disabling one of the two dispose calls.

It is more of noStoryModuleAddMethodHotDispose :D

@tmeasday
Copy link
Member

I guess what I was thinking is we are keeping the dispose for the kind (in .storiesOf) but not for the story (in .add) -- is that right?

@Gongreg
Copy link
Member Author

Gongreg commented Feb 16, 2020

That’s correct.

@tmeasday tmeasday self-requested a review February 19, 2020 22:20
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.

Haha, WFM.

@tmeasday tmeasday merged commit f9f0b82 into next Feb 19, 2020
@tmeasday tmeasday deleted the client-api-hot-refresh branch February 19, 2020 22:20
@dannyhw
Copy link
Member

dannyhw commented Aug 27, 2020

Sorry to leave comments on old PR's but these changes are part of the fast refresh fix for react native storybookjs/react-native#30 any chance we could see this in another 5.3.x patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants