react-refresh: Dependent functions/data don't trigger refresh

[lilactown]

Do you want to request a feature or report a bug?

Both/neither?

What is the current behavior?

Currently, react-refresh marks each component whose type and/or signature has changed as "dirty" and will either re-render or re-mount those components selectively.

The problem occurs when the dev tooling (webpack, parcel, etc.) loads a module that exports functions or data that are used inside of components, but aren't registered components themselves. For example, a utility function that concatenates a string:

export greet(name) {
  return `Hello, ${name}!`;
}

Changing the returned string to Yo, ${name}! would trigger this module to reload in the browser, but because components which depended on it don't reload, the old greeting will persist until the next render of each dependent component.

(BTW in actuality, some tooling will reload immediate dependents of modules that are reloaded in order to get around similar problems. You can extend the dependency chain from two to three modules, where a.js depends on b.js depends on c.js, and you will get the same result when editing c.js)

What this forces tooling to do is apply a heuristic to try and guess whether a given module should be refreshed, vs. completely restart the app in order to cause all components to re-mount and pick up any changes that wouldn't be picked up by react-refresh.

The problems with the heuristic approach is:

• each dev tool needs to implement this logic, leading to more potential for bugs
• it leads to a degradation of hot reloading capabilities based on what your module exports. not obvious at all on it's face and will lead people to twisting their code base to route around this
• It's very unfriendly to compile-to-JS languages like ReasonML, ClojureScript, etc. which might have different default semantics for what is public / private, different conventions for naming components, etc. which make it difficult to detect whether a module is "safe" to refresh

What is the expected behavior?

That components depended on newly loaded code will pick up those changes correctly, without losing state.

A potential (maybe naive?) solution to this in react-refresh is, instead of only re-rendering the components marked as dirty (due to a different type being registered), to re-render from the root while maintaining hooks state. If components' signatures have changed, then re-mount.

I've read through and kind of grok most of the code in react-refresh, but I'm not sure how this would impact the way that the reconciler currently handles the HMR stuff. This is as much of a question, as it is a request: could this be a viable solution?

I appreciate your time and energy in reading through this. I'm very excited about having first-class support for hot reloading in React, as it's been something that I've loved ever since seeing the first demo of it. I hope that this issue can help create a way to provide a consistently excellent dev experience across tools/platforms/languages!

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.11

[lilactown]

Since I'm already thinking out loud, I'll add another (potentially naive) idea: would it be possible to add another knob to the register call, forceRefresh?

This way, the tooling can add this call to components in an incremental way, for example if it detects it is using imported functions from other files, or if the user annotates the component in some way. Again, not understanding fully how it fits in with how the reconciler works, I think forceRefresh = true could be a sane default but if not, allowing people to opt in could help solve some of these corner cases without relying on fall backing to a full reload of the app.

(Editing to add more thinking out loud without spamming)

Another (or an additional) way could be to add some way of marking a component as "dirty" without necessarily registering a new type, as the component may not have reloaded but the tooling may be able to determine that the component should be refreshed.

[gaearon]

Changing the returned string to Yo, ${name}! would trigger this module to reload in the browser, but because components which depended on it don't reload, the old greeting will persist until the next render of each dependent component.

That sounds wrong and doesn't match how Fast Refresh works on React Native. I think this is a misunderstanding.

This only happens if every module "accepts" its own updates. We don't recommend to do that. Instead, the recommendation is that an update is only accepted when all exports are likely to be React components. Otherwise, the update should "propagate" upwards to the parent modules, which use the same decision procedure.

See

if (isReactRefreshBoundary(myExports)) {
  module.hot.accept(); // Depends on your bundler
  enqueueUpdate();
}

in https://github.com/facebook/react/issues/16604#issuecomment-528663101.

[lilactown]

Right, I understand that is the suggested way at the moment to work around the issue I am bringing up. I'm trying to figure out if this is a fundamental aspect of hot reloading, or if it's something that I could help improve. I should have stated in my original topic that this was based on a "naive" implementation of Fast Refresh. In my original post, I talk about the issues I see with trying to applying the workaround where we try and detect if a module is "safe to accept."

To give more context, I'm trying to implement Fast Refresh in ClojureScript, and this presents two problems:

• the tooling currently does not give you the ability to inspect the module being reloaded. You can set a global "after-load" hook to restart your app.
• By default, all ClojureScript definitions of variables and functions are public; so inspecting whether a module "exports" only components is not a good heuristic for discerning whether any of those definitions are actually used.

ClojureScript has a rich history of using hot reloading tools to create fast feedback loops. The language is designed in a way to facilitate this workflow. In the past, we have accomplished this with React by storing most of our state in global stores that don't re-define on reload, and calling React.render again on our app to restart it and get the latest definitions of any components or functions/data used within. This works great, but being able to use local state and support more concurrent-mode safe practices would be greatly preferred, which is why I'm very interested in getting Fast Refresh working well in ClojureScript.

I am asking the question: is it possible to make this problem easier to solve on the tooling side by always re-rendering the whole app, so that at least the components that depended on any functions or data that were reloaded would start using those new references immediately?

[lilactown]

One thing that I didn't understand is that webpack (maybe other JS build tools) will reload every module up to the root in the dependency tree of the edited module. In ClojureScript, tools typically stop at reloading the module that has changed and modules that directly depend on it, no further. That probably has a lot to do with why I'm struggling to make this feel ergonomic.

[lilactown]

Ok, I have thought more about this and how the differences between JS and ClojureScript modules and bundling differ. I want to try and re-contextualize my use case to see if it clarifies what I need help with.

Every ClojureScript modules has a globally unique namespace associated with it. This means that when loading a module, it doesn't need to close over references to dependent modules. It simply accesses the global namespaced variables it needs.

This means that HMR tooling only needs to reload the changed module in the usual case. Any dependents will pick up the new code when it is invoked again.

This also enables development at a REPL, instead of file watching. Re defining a function or variable means it will be automatically picked up without reloading any other code.

I haven't figured out a way to get React Refresh to work consistently under this paradigm. It seems to require me to re-register with a new type any components I want to refresh. Trying to work around this hasn't born out yet.

In a perfect world, I would be able to redefine some function, variable, component, or whatever, and call refreshApp and it would perform a trivial refresh of all components. Any components that had their type or signature changed would act as it does now.

This could be beneficial to JS too, as it could help enable refreshing applications outside the context of a bundler, or at least a bundler with different behavior than the status quo.

I am trying really hard to explain my use case, so I hope it's clearer than before. I have two questions now:

1. Is the "trivially refresh whole app" behavior something that could work with the current model?
2. Is that functionality something you would consider accepting if I were to spend the time creating a PR?

I know that y'all have probably moved on to other priorities since releasing refresh on RN, but I think this feature would have a great benefit to compile-to-JS langs in general.

[gaearon]

Is that functionality something you would consider accepting if I were to spend the time creating a PR?

Yeah sounds reasonable.

[jayenashar]

Hi there, I am trying to use react-refresh on a react-dom project and I'm hitting a very similar issue where my component js looks like this:

import React from 'react';
export default props => <div/>;

and it is also not being picked up as a component because it doesn't have a name that starts with an upper case letter, but the name in this case is _default. That is, isLikelyComponentType returns false with default exports.

Is this a case your PR will cover?

[GuskiS]

Is there any progress on this?

[gaearon]

@jayenashar

Hi there, I am trying to use react-refresh on a react-dom project and I'm hitting a very similar issue where my component js looks like this:

and it is also not being picked up as a component because it doesn't have a name that starts with an upper case letter, but the name in this case is _default

This is completely expected. If you want Fast Refresh to work, give names to your components. export default function Button, for example. This is important not only for Fast Refresh, but for other tooling that needs to differentiate components from regular functions.

@GuskiS

If there was progress, it would be on this issue. What progress would you like to see, and would you like to work to make it happen?

[GuskiS]

Is that functionality something you would consider accepting if I were to spend the time creating a PR?

Yeah sounds reasonable.

After reading this, I thought there is work being done, but not shown here.

I'm actually having different issue, but I suppose it is related. In our codebase we have classes, with static methods. When we change something in them, code is compiled(from typescript) and refetched in browser, but components that call these methods during lifecycle, are not being refreshed.

Is that issue with react-refresh or with @pmmmwh/react-refresh-webpack-plugin? Is there any way to force refresh?

[gaearon]

This is a limitation of our approach, yes, because there is no good reliable way to safely "proxy" classes.

If you refactor from static methods to exporting functions (which is pretty much the same thing) I think it should work.