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

Not working on multiple named exports #249

Closed
ArcherDs opened this issue Nov 10, 2020 · 8 comments
Closed

Not working on multiple named exports #249

ArcherDs opened this issue Nov 10, 2020 · 8 comments

Comments

@ArcherDs
Copy link

ArcherDs commented Nov 10, 2020

when the root component export multiple items , react refresh not working

here is the example
https://codesandbox.io/s/dry-flower-jq66q

If delete x from export ,then it works

Maybe this problem is related to react-refresh/babel ?

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 13, 2020

This is expected behaviour. When there are non-React related exports, we cannot safely stop the update at the module itself since we do not know if it is side effect free. We will then propagate the change up to the parent(s).

If this happens near the root, chances are that it will propagate through root and thus trigger a bail out (i.e. full refresh for web).

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 13, 2020

Also just for my understanding please describe what do you mean by not working.

@ArcherDs
Copy link
Author

ArcherDs commented Nov 16, 2020

I mean when export multiple items on the root component editing makes the web page trigger a full refresh , just like you say, forgive my poor english.
When use react with recoil , I think it's comon to export some atom state among components or pages,but this bails out react fast refresh when it's at the root position.
If this behaviour is expected ,I could only wrap the root component in an empty component to solve my problem.

@pmmmwh
Copy link
Owner

pmmmwh commented Nov 17, 2020

This is expected behaviour.

Let's say you are doing this:

index.js

import { render } from 'react-dom';
import App, { AppConstant } from './App.jsx';

render(<App />, document.getElementById('root'));

App.jsx

function App() {
  return <div />;
}

export const AppConstant = 'AppConstant';

export default App;

The issue with us automatically stopping the update at App.jsx (i.e. do not require parent update) is that we cannot guarantee that updates to AppConstant does not have any side effects (in general, only React components are considered safe to be updated and discarded since render is side-effect free). What that means for React Refresh is the update might yield an inconsistent UI state - which we absolutely does not want. For that reason, we would not construct a boundary for App.jsx, which in turn will propagate the update to the parent and let the parent re-require the child. This rises the other issue - index.js does not have any exports. Since there are no exports, we also cannot guarantee that the file is side-effect free, and cannot construct a boundary.

Now we have reached the root of the module graph - we cannot further propagate the update upwards - so unfortunately we need to bail out of HMR and do a full forced update (i.e. full page refresh).

When use react with recoil, I think it's common to export some atom state among components or pages,but this bails out react fast refresh when it's at the root position.

My suggestion to this issue would be to keep the component closest to root only export itself - so at least you would have a final safe-guard boundary near root even you have deep trees that cannot update themselves due to multiple exports.

I think it is more of how to organise code - correct me if this is wrong, but Recoil also suggest more atomic pieces of state and not having them at the root component is probably a right thing to do? There is not absolute need to export some atom state at the root component, right?

@pmmmwh pmmmwh closed this as completed Nov 17, 2020
@0xorial
Copy link

0xorial commented Mar 4, 2021

Is this project used by create-react-app? I've just spent a few hours debugging an issue that it was stopping updates if there are multiple exports, but there is no full reload or any kind of warning.

@pmmmwh
Copy link
Owner

pmmmwh commented Mar 5, 2021

Is this project used by create-react-app?

Yes, if you are on CRA v4.

I've just spent a few hours debugging an issue that it was stopping updates if there are multiple exports, but there is no full reload or any kind of warning.

This might have been a bug, I'm open to a reproduction to illustrate the issue. Note that we've also fixed quite a few things in the upcoming 0.5.0, and you can test using that version via package resolutions.

@0xorial
Copy link

0xorial commented Mar 13, 2021

I tried it just now, on a new clean project. it is pretty straightforward to reproduce:

npx create-react-app hot-test --template typescript
cd hot-test
yarn start
change App.tsx - hot update working (3 years since I am using hot reloading, still find it super awesome :) )
add export function test() {} to App.tsx - updates stop working and nothing happens (no full-page reload either).

The problem for me here is not the fact that it is not reloading, but rather that there is no errors/warnings anywhere, which make it quite puzzling and making me think that I broke smth in my project setup.

I am not sure if this is the issue with this package or with it's integration into CRA.
Alternative temporary workaround could be adding a warning in the App.tsx, so developers know what to expect.

@pmmmwh
Copy link
Owner

pmmmwh commented Jun 15, 2021

See facebook/create-react-app#11105 - this is an error upstream with CRA.

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

No branches or pull requests

3 participants