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

[Meta] Releasing this Plugin as "Fast Refresh" capable #1

Open
40 of 42 tasks
pmmmwh opened this issue Sep 16, 2019 · 41 comments
Open
40 of 42 tasks

[Meta] Releasing this Plugin as "Fast Refresh" capable #1

pmmmwh opened this issue Sep 16, 2019 · 41 comments
Labels
help wanted Extra attention is needed

Comments

@pmmmwh
Copy link
Owner

pmmmwh commented Sep 16, 2019

Ref: facebook/react#16604

This issues tracks what is missing before this plugin attains "feature-parity" with the RN implementation and is representative of the "fast-refresh" branding.

Tasks

@maisano
Copy link

maisano commented Sep 19, 2019

@pmmmwh What needs to happen in order to get this working with webpack 5? I haven't spent much time looking into the changelog there.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Sep 20, 2019

@pmmmwh What needs to happen in order to get this working with webpack 5? I haven't spent much time looking into the changelog there.

@maisano
It has nothing to do with the flow/logic, it is mostly related to hook signatures changing:

  1. The afterResolve now returns resolvedData with a field createData - which is the equivalent of webpack 4 resolveData. The hook is also switched to a series bail hook so we can't return the modified data.
  2. The require hook in mainTemplate now switched to return chunk context instead of the actual chunk. To effectively detect entry modules, we now have to get iterable entry modules from the chunkGraph then loop them through.
  3. Direct references to the normal module loader (which I used to detect hot context here) is completely removed and we now have to get it via webpack.NormalModule.getCompilationHooks(compilation).loader.

In fact - I have already implemented these changes, locally. But because webpack 5 is still in alpha, I am not sure if releasing them at this stage is a good move.

Speaking of versions, I am still debating on the proper range of webpack versions to support. It would be easiest to only support webpack 4+, but I do understand that webpack 3 is still being used by a lot of people.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Sep 20, 2019

@gaearon
I have been working on the error box integration for a few days now - and have got to a point where I would like your input on a few things:

  • I was trying to integrate with react-error-overlay, like how react-dev-utils did for CRA. However, I read from here that the overlay is going to get a rewrite soon - mind tell me a little bit more about that?
  • (edited to better phrase the question) I have made recovery from runtime errors and build-time errors both working, utilizing the exact same approach from react-dev-utils and webpack-dev-server's overlays.
    • For build errors, I haven't found an alternative to having a socket server in-place, whether it is from webpack or it is built into the plugin. Initially, I achieved this by building a socket server into the plugin because I didn't want to rely on WDS - but thinking about it more, I probably was wrong, and that would probably make things more complicated than what it should be. Not to mention that addressing the needs of every webpack + React user is probably unrealistic. My new proposal to make this work:
      1. Baseline - allow zero-configuration experience with WDS
      2. For advanced users, provide an express style middleware that will work with webpack-dev-middleware which creates an endpoint that does the same thing as WDS's stats emitter.
    • For runtime errors, if we go for a similar experience that react-error-overlay provides, a sourcemap (cheap-module-source-map in particular) have to be generated. I am not sure if enforcing this is a good idea, especially when this have implications in using browser debuggers. I can still provide insights on what the error is about and the stack, just not as elegant as having the source visible for debugging.

@pmmmwh pmmmwh pinned this issue Sep 20, 2019
@gaearon
Copy link

gaearon commented Sep 21, 2019

Do you two wanna do a video call next week with me? Might be easier to talk through all issues.

@maisano
Copy link

maisano commented Sep 22, 2019

Sounds good. I'm on EST (GMT-4) and generally pretty flexible, so let me know what works for you all.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Sep 22, 2019

Do you two wanna do a video call next week with me? Might be easier to talk through all issues.

Sounds good to me too. I am on GMT+8 and also pretty flexible. What time will work for you Dan?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Sep 23, 2019

I have been working on the error box integration for a few days now

My progress is tracked on this branch. It is pretty rough though.

@bvaughn
Copy link

bvaughn commented Sep 30, 2019

Did the three of you ever end up talking?

If I was looking to lend a hand here, what would be the most impactful thing to look into?

Maybe it would be helpful for two or three of us to chat? (Assuming you'd like a hand.)

@maisano
Copy link

maisano commented Sep 30, 2019

👋 @bvaughn – we haven't met yet–I have some time this week if we want to try. Are you on PST?

@pmmmwh – assuming this is you, can you open your DMs? It'd be easier to coordinate there.

@bvaughn
Copy link

bvaughn commented Sep 30, 2019

Hi 👋 I am located in California, so yeah PST. Happy to chat for a bit to ramp up context if that would be helpful.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Oct 1, 2019

Did the three of you ever end up talking?

If I was looking to lend a hand here, what would be the most impactful thing to look into?

Maybe it would be helpful for two or three of us to chat? (Assuming you'd like a hand.)

Nope - we haven't talked yet. I definitely would like a hand 😃 . It would be great if you can provide more context on the error-box experience @bvaughn .

👋 @bvaughn – we haven't met yet–I have some time this week if we want to try. Are you on PST?

@pmmmwh – assuming this is you, can you open your DMs? It'd be easier to coordinate there.

@maisano Yep that's my long abandoned Twitter account - I have opened my DMs.

@bvaughn
Copy link

bvaughn commented Oct 1, 2019

@maisano Want to start a thread between the three of us on Twitter?

@charrondev
Copy link

charrondev commented Oct 31, 2019

Just wanted to say that I started testing it our setup here:

vanilla/vanilla#9509

Seems to be working better than react-hot-loader ever did. Mainly because we mount a few different page (many of which are named export making manually wrapping them in hot() particularly tedious and ugly). Auto-registration is awesome!

Has there been any further discussion about implementing the error overlay? The discussion kind of trailed off. I'm available to help with that if you want assistance.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 1, 2019

Just wanted to say that I started testing it our setup here:

vanilla/vanilla#9509

Seems to be working better than react-hot-loader ever did. Mainly because we mount a few different page (many of which are named export making manually wrapping them in hot() particularly tedious and ugly). Auto-registration is awesome!

Has there been any further discussion about implementing the error overlay? The discussion kind of trailed off. I'm available to help with that if you want assistance.

Hey! Thanks for trying out the plugin. I'm happy that it worked well!

Regarding the error overlay, we did start a conversation on Twitter on that (it is technically also tracking the progress). I haven't got all the missing pieces - yet, but here are some things that have to be addressed:

  • The current plugin does not work reliably when components' refresh boundaries get mutated. I am working on that currently.
  • The current overlay requires several dev server middlewares served via react-dev-utils to deliver the complete experience. While that is cool and provides good DX, it is a bit too much of a lock in given that this plugin's scope will be much wider than that of create-react-app. I haven't figured out ways to deliver a similar experience yet.
  • The code for the error overlay is pretty dated, and will probably need some refactoring to be future proof (especially when webpack@5 is going to remove polyfills for Node.js internals: url). As much as I wanted to fork it and start working right away, this will need some further discussion.

Oh, and to address your fears from your PR - the public facing API of the plugin/loader probably wouldn't have breaking changes. I try to be careful with the internals, but to handle error box I believe we might have to dig into more webpack hooks, and that might cause some change in behaviour.

@gaearon
Copy link

gaearon commented Nov 1, 2019

For an error experience, I'd be ok if:

  • The default was super simple, just like an iframe overlay that shows an error message with stack
  • There was a way to specify a custom package which the plugin would call instead
    • CRA and others could use it to integrate with React Error Overlay

Makes sense?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 1, 2019

For an error experience, I'd be ok if:

* The default was super simple, just like an iframe overlay that shows an error message with stack

* There was a way to specify a custom package which the plugin would call instead
  
  * CRA and others could use it to integrate with React Error Overlay

Makes sense?

Hey Dan! Thanks for the prompt reply - that is definitely going to help.
Mind if I grab you for a PR review in the near future?

I think I can also track an issue on the error overlay in the CRA repo later after I'm done with the base case - and see what needs be done (both sides) to integrate with the implementation.

@gaearon
Copy link

gaearon commented Nov 2, 2019

Sure I'd be happy to review. In fact let me know when you think this is ready for a DX review pass. I can write some notes on what works well and what seems missing.

@charrondev
Copy link

Going back on the error overlay:

  • What are the types of errors that we want to be catching and displaying here?
    • Hot reload errors? (Eg. hot reload failed)
    • Webpack build errors?
    • React errors that would normally just crash/unmount the component stack?
  • In an application with more than one mount point is the overlay meant to take up the whole screen or just part of the screen that the component was mounted at?

I'm not 100% convinced that an error overlay needs to be part of the fast refresh implementation, especially if it would require using wrapping components in some sort of wrapper like react-hot-loader did. I know @gaearon had mentioned an error overlay in facebook/react#16604 but to me this has a ton of utility even without that being built in.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 2, 2019

Going back on the error overlay:

* What are the types of errors that we want to be catching and displaying here?
  
  * Hot reload errors? (Eg. hot reload failed)
  * Webpack build errors?
  * React errors that would normally just crash/unmount the component stack?

* In an application with more than one mount point is the overlay meant to take up the whole screen or just part of the screen that the component was mounted at?

I'm not 100% convinced that an error overlay needs to be part of the fast refresh implementation, especially if it would require using wrapping components in some sort of wrapper like react-hot-loader did. I know @gaearon had mentioned an error overlay in facebook/react#16604 but to me this has a ton of utility even without that being built in.

To my understanding, there are 3 types of errors: build time (i.e. compilation), run time (i.e. JS/React, not caught with Error Boundaries) and integration (e.g. HMR, socket integration, dev server) error. The error integration would have to render the first 2 types gracefully for it to pass - the third type we can either fail and force page reload, or just stop propagation. Based on this assumption, we wouldn't need to wrap components like react-hot-loader. We might have to wrap console.error or console.warn, which is what used by invariant used within React, though. For multiple mount points - it will probably occupy the whole screen.

For a baseline zero-configuration setup, I argue that having the error integration would be a plus - it makes the error obvious for the user, instead of having a "blank screen of death". It provides insight on at least which part of the app is broken and places to search for in order to fix that. However, for advanced users, I think an option to disable the error integration would be something worth looking into - like if someone is force enabling this plugin in a production build.

Does that make sense to you? @charrondev

@charrondev
Copy link

charrondev commented Nov 4, 2019

Thanks for the clarification.

I took a quick look trying to find existing integrations w/ react-error-overlay and found one error-overlay-webpack-plugin, which looks polished but I couldn't get it to work at all.

I'm not going to have any more time today to dive into it, but trying to get your error overlay branch also isn't working in our project, does it work at all on your end?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 4, 2019

Thanks for the clarification.

I took a quick look trying to find existing integrations w/ react-error-overlay and found one error-overlay-webpack-plugin, which looks polished but I couldn't get it to work at all.

I'm not going to have any more time today to dive into it, but trying to get your error overlay branch also isn't working in our project, does it work at all on your end?

If you add devtool: 'cheap-module-source-map' to your webpack configuration, it should work as expected. I did update the branch though, to make HMR detection more robust.

@gaearon
Copy link

gaearon commented Nov 4, 2019

Can we start with just build errors for V1?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 4, 2019

@gaearon
Is there a way for the plugin to notify react-refresh that exports are mutated and probably need to remount? I am trying to handle the case where exports are mutated: I can correctly detect that exports have changed via HMR, and even capture the previous/next modules, but since their corresponding signatures did not change, the updates won't get processed (unless the mutation only included classes, which will always remount). In Metro, this is handled by re-evaluating the parent when this happens, but I am not sure if this can be done in Webpack.

Consider the following example:

import React from 'react';

function ComponentA() {
  return <span>Component A</span>;
}

function ComponentB() {
  return <span>Component B</span>;
}

export default ComponentA;

If I update the last line from ComponentA to ComponentB, it should have "refreshed". But since the signatures for functions are calculated by the Babel plugin first, the signature registration through exports won't get recognized and thus there is no change in signatures. In metroOr in that case we should bail out and do a full refresh?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 4, 2019

Can we start with just build errors for V1?

Actually, I just (few hours ago) pushed some changes to the feat/error-overlay branch. It is using react error overlay, and will require some cleanup, but I would consider the general error experience for the usual use case (both build/runtime) done.

If you don't mind - @gaearon can you can clone it and do a DX review?

I still need to work on swapping out the error overlay, which probably will take me a few days, but will probably keep somewhat API-compliant to the current approach to ease integration with CRA.

@gaearon
Copy link

gaearon commented Nov 4, 2019

In Metro, this is handled by re-evaluating the parent when this happens, but I am not sure if this can be done in Webpack.

I think we should just force a full page reload for now. It would indeed be nice if webpack provided a way to do this but I don't think it's currently possible. We can definitely detect it, see the "is boundary invalidated" code in Metro.

@gaearon
Copy link

gaearon commented Nov 4, 2019

Another case you'll notice affected by this is changing component from exporting a class to a function.

@gaearon
Copy link

gaearon commented Nov 4, 2019

Regarding react-error-overlay. Are there any concerns about it relying too much on CRA assumptions? Like I'm not sure how stack symbolication works but does it load the correct .map file when there's multiple entry points etc?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Nov 4, 2019

In Metro, this is handled by re-evaluating the parent when this happens, but I am not sure if this can be done in Webpack.

I think we should just force a full page reload for now. It would indeed be nice if webpack provided a way to do this but I don't think it's currently possible. We can definitely detect it, see the "is boundary invalidated" code in Metro.

I did reference that, and have already implemented the check/bail out for full refresh 😺 .

Another case you'll notice affected by this is changing component from exporting a class to a function.

Yes, basically any combinations of prev/next with a function would hit that.

Regarding react-error-overlay. Are there any concerns about it relying too much on CRA assumptions? Like I'm not sure how stack symbolication works but does it load the correct .map file when there's multiple entry points etc?

I think it works with multiple entry points. The main assumptions to me that would fail in any projects not from CRA would be the custom dev server middlewares and cheap-module-source-map (devtool). It also won't work with resource queries, custom socket implementations nor webpack under proxy servers, but I would consider them as advanced use cases anyhow, and thus will definitely need custom work.

@charrondev
Copy link

I think it works with multiple entry points. The main assumptions to me that would fail in any projects not from CRA would be the custom dev server middlewares and cheap-module-source-map (devtool).

I'll give the branch another test tomorrow now that it's updated. I'm using it multi-compiler setup w/ multiple entrypoints per compiler.

I did reference that, and have already implemented the check/bail out for full refresh 😺 .

I'll double check again as well, but that seemed to be working form my testing as well, and was respecting the hotOnly flag from the webpack dev middleware.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Dec 6, 2019

Hey all - Thanks for participating in the conversation 😺!
I am happy to announce v0.1.0 is out now 🎉

@swashata
Copy link

Hi @pmmmwh, Thank you for your awesome work here. I would like to integrate this with https://wpack.io (https://github.com/swashata/wp-webpack-script) (A CRA like tooling for WordPress) and there I am proxying WordPress site through browser-sync with Webpack Dev Middleware and Webpack hot middleware.

I see in your todo list that we need to have express like middleware to cater advance WDM users. Since I am one of those users, I would like help in anyway I can to make this possible. Any tips on where to get started?

@pmmmwh
Copy link
Owner Author

pmmmwh commented Dec 28, 2019

Hi @pmmmwh, Thank you for your awesome work here. I would like to integrate this with https://wpack.io (https://github.com/swashata/wp-webpack-script) (A CRA like tooling for WordPress) and there I am proxying WordPress site through browser-sync with Webpack Dev Middleware and Webpack hot middleware.

I see in your todo list that we need to have express like middleware to cater advance WDM users. Since I am one of those users, I would like help in anyway I can to make this possible. Any tips on where to get started?

You can checkout #23 - the solution shown there is not perfect (because of entrypoint ordering issues). I was thinking to draft an API to allow custom entry ordering, but afaik WHM seems to be the only know hot client using window to inject, so another way to prevent complicating things is to custom handle detection of that entry in the plugin. To do that you can see the section commented as inject entrypoints in the index.

@phaistonian
Copy link

@pmmmwh So, a solution for this, i.e support for hot middleware, is or will on its way?

ps: Happy 2k20!

@mattfwood
Copy link

Really appreciate the work you're doing here and looking forward to using this.

I was looking at #36 and was also wondering if things like server-side rendering support will be included in this becoming officially "fast refresh" capable?

I'd be happy to help with this, so far I've been trying to use this with Next.js but after trying to configure it for a few hours I can't get state in hooks to persist between renders, even though it's using webpack and HMR on the client side.

I do have it ignoring webpack compiles on the server side, since otherwise it tries to access window

@pmmmwh
Copy link
Owner Author

pmmmwh commented Feb 21, 2020

I was looking at #36 and was also wondering if things like server-side rendering support will be included in this becoming officially "fast refresh" capable?

SSR support will probably be something that have to be case by case unfortunately. I don't have a lot of knowledge on specifically how SSRed components are being hydrated, so any insights will help.

I'd be happy to help with this, so far I've been trying to use this with Next.js but after trying to configure it for a few hours I can't get state in hooks to persist between renders, even though it's using webpack and HMR on the client side.

For Next.js it's difficult because their Webpack setup is indeed very complicated. Next's maintainers have shown interest in adding this, so hopefully after we tackle all the prerequisites it can be available to everyone in that ecosystem.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Jul 8, 2020

Roadmap to v1 Stable

@pmmmwh pmmmwh mentioned this issue Jul 8, 2020
@gaearon
Copy link

gaearon commented Jul 8, 2020

SSR support will probably be something that have to be case by case unfortunately. I don't have a lot of knowledge on specifically how SSRed components are being hydrated, so any insights will help.

I wouldn't expect any differences when SSR is on.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Jul 8, 2020

I wouldn't expect any differences when SSR is on.

Initially I was expecting some difference because I wasn't sure how SSR build setups work, and the implementation here was quite rough too.

With the latest iteration I think there shouldn't be any difference in Fast Refresh compatibility, but there might still be some difference in the experience due to the libraries are being used (e.g. React.lazy v.s. loadable-components) and how builds are being set up (custom SSR setups will require more config).

@pmmmwh pmmmwh changed the title Releasing this Plugin as "Fast Refresh" capable [Meta] Releasing this Plugin as "Fast Refresh" capable Sep 3, 2020
@bdanzer

This comment has been minimized.

@pmmmwh
Copy link
Owner Author

pmmmwh commented Jan 2, 2021

@bdanzer Please see #226 (comment)

@nyngwang
Copy link

Post here for attention: I believe error recovery is now completely broken with webpack-dev-server@5, see #806 for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

11 participants