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 by saving the preview frame URL as the story changes. #2349

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

tmeasday
Copy link
Member

Sometimes when there are problems with HMR, we end up doing a
hard-reload of the preview iframe, whilst leaving the main window.

As the preview iframe didn't change its URL ever, this led to problems
where the old (usually original) story was loaded in such circumstances.

Issue:

See #614 and #1328 -- and possibly some more, I'll have a hunt through the issue list.

What I did

use pushState to change the preview's URL as stories change.

How to test

See the repro on #614

Is this testable with jest or storyshots?

No

Does this need a new example in the kitchen sink apps?

No

Does this need an update to the documentation?

No

If your answer is yes to any of these, please make sure to include it in your PR.

Sometimes when there are problems with HMR, we end up doing a
hard-reload of the preview iframe, whilst leaving the main window.

As the preview iframe didn't change its URL ever, this led to problems
where the old (usually original) story was loaded in such circumstances.

See #614 and #1328
@tmeasday
Copy link
Member Author

I'm not sure where we are at in the release cycle; this could go straight out if you prefer.

@codecov
Copy link

codecov bot commented Nov 22, 2017

Codecov Report

Merging #2349 into release/3.3 will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #2349      +/-   ##
===============================================
- Coverage         22.8%   22.75%   -0.06%     
===============================================
  Files              326      326              
  Lines             6752     6767      +15     
  Branches           836      845       +9     
===============================================
  Hits              1540     1540              
- Misses            4601     4607       +6     
- Partials           611      620       +9
Impacted Files Coverage Δ
app/react/src/client/preview/init.js 0% <0%> (ø) ⬆️
app/vue/src/client/preview/init.js 0% <0%> (ø) ⬆️
app/angular/src/client/preview/init.js 0% <0%> (ø) ⬆️
app/react/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/utils.js 0% <0%> (-53.58%) ⬇️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 51.85% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/error_display.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 9.3% <0%> (ø) ⬆️
... and 32 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 462a8d9...6638f5c. Read the comment docs.

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Great!

@Hypnosphi Hypnosphi merged commit 776f3a6 into release/3.3 Nov 22, 2017
@Hypnosphi
Copy link
Member

If we decide to have it on master, we can try the cherry-picking approach I proposed for future releases

@Hypnosphi Hypnosphi deleted the 1328-fix-hmr-failure branch November 22, 2017 10:28
@shilman shilman changed the title Save the preview frame URL as the story changes. Fix HMR by saving the preview frame URL as the story changes. Nov 22, 2017
@RobinMalfait
Copy link

I am going to be so happy once this is released 👍

@Hypnosphi
Copy link
Member

@RobinMalfait this is released as 3.3.0-alpha.4

tmeasday added a commit that referenced this pull request Jan 3, 2018
This didn't apply at all cleanly as it involved adding code to each framework's `init.js`, which was removed in the refactor.

Here we do better; we simply abstract the job of URL<->redux store syncing to the core library.
tmeasday added a commit that referenced this pull request Jan 3, 2018
This didn't apply at all cleanly as it involved adding code to each framework's `init.js`, which was removed in the refactor.

Here we do better; we simply abstract the job of URL<->redux store syncing to the core library.
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