Reload html on html asset change #90
Conversation
Raathigesh
changed the title
Raathigesh
changed the title
|
I feel like the README changes should go in a different Pull Request... this looks a bit messy. |
|
Agreed. That's a mistake. Will fix! |
src/HMRServer.js
Outdated
| emitReload() { | ||
| let msg = JSON.stringify({ | ||
| type: 'reload' | ||
| }); |
There was a problem hiding this comment.
Why would you stringify a JSON literal right after creating it o_0? I’m sure it’s not a huge performance penalty, but it feels a bit wrong
There was a problem hiding this comment.
We are sending the string through the socket. See line 45.
There was a problem hiding this comment.
Ya exactly, so why not just keep it a string the whole time?
So instead of:
let msg = JSON.stringify({
foo: 'bar'
})We can do:
let msg = `{
foo: ‘bar’
}`You get me? Like in the original, we are creating a new JavaScript object using JSON literal notation, and then passing it into JSON.stringify as a parameter, and then taking the outputted string and assigning it to the variable msg.
What I’m saying, is to just make it a string in the first place.
There was a problem hiding this comment.
agreed with @davidnagli, if parcel is aiming to be fast, we can't spent time on stringify'ing object, even with native JSON.stringify
There was a problem hiding this comment.
@davidnagli It's a mistake to optimize prematurely at the expense of code quality.
There are other reasons to prefer the JSON.stringify version. For example, webstorm IDE support will be able to detect the keys and values being used, but can't for string literals. When refractoring code, typos become harder to detect, since you don't get syntax highlighting. And if a refactor wants to interpolate some data into the object, you'll have to revert it to JSON.stringify anyway.
That said, yeah, you could do `{type: 'reload'}` if you really wanted to, but there is zero chance that a reload emission would impact performance in any measurable way.
There was a problem hiding this comment.
Yeah, JSON.stringify is not going to affect perf. leave it.
src/Bundler.js
Outdated
| @@ -208,7 +214,11 @@ class Bundler extends EventEmitter { | |||
| // Emit an HMR update for any new assets (that don't have a parent bundle yet) | |||
| // plus the asset that actually changed. | |||
| if (this.hmr && !isInitialBundle) { | |||
| this.hmr.emitUpdate([...this.findOrphanAssets(), ...loadedAssets]); | |||
| if (didProcessHtmlAsset) { | |||
There was a problem hiding this comment.
Can we do the detection of HTML assets in the HMRServer instead of here? All of the updated assets are already passed to emitUpdate so it should be easy to do the detection in there.
There was a problem hiding this comment.
I'll do that. thanks
|
@devongovett I did think about that but couldn't find a way to identify the current HTML file. We could restrict the reload only when the entry file is an HTML file and reload only when then entry file changes. What do you think? |
|
Has this been published to npm yet? I just installed parcel and changing html file still doesn't reload the page. EDIT: nevermind, it works only if I include at least one file. As soon as I included a css file, it started working. |
|
I have the same problem as @dumptyd. I often like to test things in a single html file and parcel seemed perfect to provide autoreload functionality for this case. Is it possible to activate the reload functionality for this case as well? |
|
@therealshark please open up a new feature request for this Sent with GitHawk |
Fixes #83