Skip to content

feat: Add support for refreshing hmr on failure#182

Merged
shellscape merged 12 commits intoshellscape:masterfrom
bebraw:feat-hot-fallback
Oct 15, 2020
Merged

feat: Add support for refreshing hmr on failure#182
shellscape merged 12 commits intoshellscape:masterfrom
bebraw:feat-hot-fallback

Conversation

@bebraw
Copy link
Collaborator

@bebraw bebraw commented Aug 24, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

This PR adds support hot: 'refresh-on-failure'.

Technically we could reduce hot and liveReload to a single enum with three options as you should never have hot and liveReload enabled anyhow but that change likely goes beyond this PR.

Open questions

  • What's a good way to test the functionality? Should I add a new integration test for this or how to make sure we trigger the functionality the right way?

@bebraw
Copy link
Collaborator Author

bebraw commented Oct 1, 2020

@shellscape Let me know when we can push this one further so I can improve as needed.

@bebraw bebraw force-pushed the feat-hot-fallback branch from 197259b to 2318617 Compare October 10, 2020 12:42
@bebraw bebraw changed the title feat: Add support for refreshing hmr on failure (work in progress) feat: Add support for refreshing hmr on failure Oct 10, 2020
@bebraw
Copy link
Collaborator Author

bebraw commented Oct 10, 2020

@TrySound This one is good to check. I'll have to try it against an actual project to make sure the refresh logic works.

@bebraw bebraw force-pushed the feat-hot-fallback branch from a31b623 to ff02a85 Compare October 11, 2020 09:57
It looks like that requires a little different kind of check.
@bebraw
Copy link
Collaborator Author

bebraw commented Oct 11, 2020

I set up a tiny project where to test this and found out that it requires a little different kind of check to refresh correctly. Now it emits an error (below) from HMR plugin after refreshing but at least the functionality works.

Screenshot 2020-10-11 at 12 56 54

Should I include the example to this project? I could set up a directory where to push it.

},
"keywords": [],
"author": "Juho Vepsäläinen",
"license": "MIT",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if you want me to change any of this. I can also drop some of the fields.

"pre-commit": "^1.2.2",
"puppeteer": "^3.0.2",
"webpack": "^4.43.0",
"react-refresh": "^0.8.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one seems a necessary bad due to how the client looks it up. I suppose ideally it would live only within the example.

"puppeteer": "^3.0.2",
"webpack": "^4.43.0",
"react-refresh": "^0.8.3",
"webpack": "^4.44.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can revert the version update. I made it to use a fairly new version as otherwise react-refresh won't work. Likely we should find a way to test against 5 as well.

@shellscape
Copy link
Owner

Let's move the examples under recipes which already exists?

@bebraw
Copy link
Collaborator Author

bebraw commented Oct 14, 2020

Let's move the examples under recipes which already exists?

Sure, I'll move it there. That's a good idea. 👍

@shellscape
Copy link
Owner

Forgot to mention, there's also a recipes index on the readme we'll need to update

@bebraw
Copy link
Collaborator Author

bebraw commented Oct 14, 2020

Forgot to mention, there's also a recipes index on the readme we'll need to update

Ok, done.

Recipes aren't linted anyhow.
@shellscape
Copy link
Owner

Looks good, good to merge?

@bebraw
Copy link
Collaborator Author

bebraw commented Oct 14, 2020

Looks good, good to merge?

Yeah, let's merge and publish. Feel free to squash and push out a minor release with the feature. 👍

@TkDodo
Copy link

TkDodo commented Oct 15, 2020

@bebraw I checked out your demo and it works nicely:

  • changes in App.jsx hot-reload as expected
  • changes in another.js cause a failure, which causes a re-load 🎉

Screenshot 2020-10-15 at 16 16 15

@shellscape shellscape merged commit ff97695 into shellscape:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants