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

Add --reload flag #2677

Closed
wants to merge 4 commits into from
Closed

Add --reload flag #2677

wants to merge 4 commits into from

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Feb 23, 2019

↪️ Pull Request

Add a flag to always fully reload the page.

Closes #289

After merging:

🚨 Test instructions

parcel index.html --reload will fully reload the page if some JS is changed (instead of merely re-executing it).

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@mischnic mischnic marked this pull request as ready for review February 24, 2019 12:35
@devongovett
Copy link
Member

Is this an alternative to #2676?

@mischnic
Copy link
Member Author

Is this an alternative to #2676?

I'm not sure if this is actually useful with that other one merged.
I somewhat wrote about that in the issue: #289 (comment)

@mischnic
Copy link
Member Author

Is this an alternative to #2676?

Actually, that one doesn't work in 100% of cases.

@devongovett
Copy link
Member

I do think page reloading should probably be the default unless you accept the HMR update, as proposed by #2676. HMR not working seems to be more common than working. You need to write your code in a specific way in order for HMR to work most of the time. I think if you don't opt in then we should just reload the page by default.

@mischnic
Copy link
Member Author

mischnic commented Feb 28, 2019

You need to write your code in a specific way in order for HMR to work most of the time.

With current React/Preact code, it works perfectly as it is right now (re-executing JS). What would I have to do the reenable that?

ReactDOM.render(<App/>, document.body);

if(module.hot){
	module.hot.accept(() => "dummy");
}

@devongovett
Copy link
Member

With current React/Preact code, it works perfectly as it is right now

Sometimes, in very simple apps, yes. But if you do anything more complicated, like have global state in redux, etc., then it won't always work properly.

Yeah, you'd need to do what you said above. You can just module.hot.accept() as well - the function isn't required.

I believe this is also required by webpack, or it will just reload the page, so we'd be matching their behavior.

@mischnic
Copy link
Member Author

mischnic commented Mar 3, 2019

I believe this is also required by webpack, or it will just reload the page, so we'd be matching their behavior.

👍 -> #2676

@mischnic mischnic closed this Mar 3, 2019
@mischnic mischnic deleted the reload-flag branch March 3, 2019 10:18
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.

Page reload doesn't work with lit-element 🙋 Regular Browser Reload on File Save
3 participants