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

FIX HMR #11709

Merged
merged 2 commits into from
Jul 29, 2020
Merged

FIX HMR #11709

merged 2 commits into from
Jul 29, 2020

Conversation

ndelangen
Copy link
Member

Issue: #11602

What I did

I don't know what changed, but this seems to fix the issue. Let's discuss!

How to test

  • Open a storybook
  • change a .stories.js file
  • check if HMR occurs rather then page-refresh

@shilman
Copy link
Member

shilman commented Jul 28, 2020

Oohh cc @tmeasday

@ndelangen
Copy link
Member Author

test:

  • change preview.js
  • expect a full reload
  • change story
  • expect a HMR

@ndelangen
Copy link
Member Author

I verified, that changing preview.js causes a page refresh, changing the stories (or descending files causes a HMR)

@tmeasday
Copy link
Member

tmeasday commented Jul 29, 2020

Ahh, OK this makes sense.

So this change kind of reintroduces the problem that we were working around in #11431, but there is a bit more subtlety than I realised.

There are 3 scenarios of interest here (sorry @ndelangen I only thought of 2):

  1. User changes an actual story file or dependent.
  2. User changes a preview.js that DOES NOT call configure, such as in our OfficialSB. (This is the typical&recommended setup now).
  3. User changes a preview.js that calls configure()

In case 3. as the configure() call gets passed the module it will HMR. We no longer clear decorators at all so I guess any decorators in preview.js (which there'll be less of now, I suppose) will "double up". Of course we don't let decorators run twice now anyway so this is less of a problem too, it'll probably just lead to warnings.

(Also you cannot "unset" parameters either, but that was an existing bug in 5.3 that doesn't seem to have been a big problem).

WDYT about this @shilman / @ndelangen? Should we try and be more tricky or is this OK?

@shilman
Copy link
Member

shilman commented Jul 29, 2020

I think we've deprecated the configure call at this point, so we can afford to optimize for the main.js stories case now.

@tmeasday
Copy link
Member

Agreed, so I think we are OK to merge this.

@ndelangen ndelangen merged commit 9d2b53d into next Jul 29, 2020
@ndelangen ndelangen deleted the fix/11602-hmr-not-working branch July 29, 2020 08:53
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.

3 participants