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 option for hard reload on non-CSS assets #443

Closed
wants to merge 309 commits into from

Conversation

mattdesl
Copy link
Contributor

With this new --reload option, parcel will trigger hard page reloads on JS and HTML assets, but should continue to inject updates with CSS assets.

See discussion here:
#289

Personally I feel this should be the default rather than opt-in (HMR is not even working currently) but to each their own. 🍻

tb and others added 11 commits December 28, 2017 00:28
* patch(hmr): use hostname for WS instantiation

* test(hmr): add hostname shim for testing

* Move location shim somewhere common
* Add url dependency for serviceWorker.register calls

* extract matchesPattern helper

* sync SW regexp with matchesPattern arg
* handle app store url scheme

* Update scheme regex
…#340)

* Fix: Error when scss file is empty parcel-bundler#327

* Return empty string when code is empty

* Handling empty files

* No need to load in mightHaveDependencies()

* mightHaveDependencies doesn't run if this.contents is empty.
* web worker support

* fix lint error
if (containsHtmlAsset) {
const isReload = this.options.reload;
const isReloadAsset = assets.some(asset => {
return isReload ? asset.type !== 'css' : asset.type === 'html';

Choose a reason for hiding this comment

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

It seems that the isReload condition check can move out of the some loop

@devongovett
Copy link
Member

devongovett commented Dec 31, 2017

Is this option really needed? Could you just hook into the HMR system yourself to do a reload?

in your main JS file:

if (module.hot) {
  module.hot.accept(function () {
    window.location.reload();
  });
}

@brandon93s
Copy link
Contributor

+1 on the "is this needed" front. The reload suggestion above works, and building your code to perform a DOM "upsert" (see issue suggestions) also works.

This does however, seem to be desired by quite a few.

if (containsHtmlAsset) {
const isReload = this.options.reload;
const isReloadAsset = assets.some(asset => {
return isReload ? asset.type !== 'css' : asset.type === 'html';
Copy link

Choose a reason for hiding this comment

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

Actually I hope the modification for styles can also reload the page, because it is related to DOM.
So I am thinking would it be better to be like isReload ? asset.type === 'js' : (asset.type === 'html' || asset.type === 'css')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like you want reload to happen in any situation, so:

const isReloadAsset = isReload ? true : assets.some(a => a.type === 'html');

During the PR I assumed it was desirable to have CSS apply without page reloads, since it doesn't depend on JavaScript state and the like. But because it's using import, maybe it runs into the same problems, and it's better to just hard reload the page?

Copy link

Choose a reason for hiding this comment

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

Oh yes. Maybe hot reload every time is more like what I need 😆

@xu3u4
Copy link

xu3u4 commented Jan 2, 2018

Agree.
The HMR only appends the new content, which means the old content still exists.
So the number of times we save our js module, the same number of different old version script exists. And when the corresponding action is triggered, it runs through all the appended scripts, from the oldest to the newest.
I think it is a little bit odd.
Adding --reload might be a quicker and safer solution.

@OrganicPanda
Copy link

Looks like this has been approved but has fallen in to conflict. Would love to see this merged though - @mattdesl do you have any time to fix the conflicts?

@filipesabella
Copy link

hey @mattdesl if you feel you can't find time to see this one through, I'd be happy to take on solving the conflicts.

@mattdesl
Copy link
Contributor Author

See the latest comment in the original discussion thread - it sounds like a cleaner fix than the command line flag in this PR.

#289 (comment)

@filipesabella
Copy link

filipesabella commented May 15, 2018

Ah I see, thanks for the reply.

Well, to be honest, I get browser freezes with HMR on with the two apps I'm working on.
One is a vanilla typescript+react+redux and the other one is a canvas thing.
As it stands now I have to disable HMR and refresh the browser every time, which's a little unfortunate.

As much as I like the idea of HMR, I'd love to just have the option you introduced in this PR, but I understand that it doesn't match the project's vision.

@mattdesl
Copy link
Contributor Author

mattdesl commented May 15, 2018 via email

@OrganicPanda
Copy link

Same here! HMR gives me limited confidence so I end up refreshing manually anyway haha

@marcushellberg
Copy link

I find myself needing this in projects using web components. Calling customElements.define() fails if the element has already been defined.

@devongovett
Copy link
Member

Due to move to monorepo which rewrote the git history, this PR was automatically closed. Please open a new PR against master with your changes. You can do a diff against the 1.x branch to see them. Sorry about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HMR Hot Module Reloading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet