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

fix($hmr): Target correct link tags when hot reloading. #24

Merged
merged 3 commits into from
Jul 17, 2017
Merged

fix($hmr): Target correct link tags when hot reloading. #24

merged 3 commits into from
Jul 17, 2017

Conversation

phyllisstein
Copy link
Contributor

Update the logic for comparing link tags' URLs so that the correct pathname is matched against.

Fixes #23.

Update the logic for comparing link tags' URLs so that the correct pathname is matched against.

#23
@phyllisstein
Copy link
Contributor Author

And here she is in action:

screenflow

@faceyspacey
Copy link
Owner

faceyspacey commented Jul 16, 2017

Excellent! Question: what's the browser support for URL? I've actually never used that.

Also, does this solve this issue:
#25

@phyllisstein
Copy link
Contributor Author

phyllisstein commented Jul 16, 2017

Fair question! Looks like support is solid except in IE: http://caniuse.com/#feat=url. For what it's worth I can't get HMR working at all in IE---webpack-hot-middleware throws errors about a missing EventSource object---so I'd argue that it's up for grabs how much time and energy should go into supporting it (especially when there's a URL polyfill extant).

As written this will not fix #25, but if you want to go the URL route it's a pretty quick fix---just have to add the base URL as the constructor's second argument. I'll take a look at it this afternoon.

If the Webpack publicPath contains "http:" or "https:", treat it as the
base URL for newly-loaded
assets. Otherwise, continue to use the current window's location.

Fixes #25.
var origin = document.location.protocol + '//' + document.location.hostname + (document.location.port ? ':' + document.location.port: '');
var newHref = origin + publicPath + outputFilename
var styleSheets = document.getElementsByTagName('link');
var newHref = publicPath.match(/https?:/g) ? new URL(outputFilename, publicPath) : new URL(outputFilename, window.location);
Copy link

Choose a reason for hiding this comment

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

is the publicPath lost in case it is relative?
new URL(outputFilename, window.location); => new URL(publicPath + outputFilename, window.location); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Wasn't necessary with my setup because publicPath was /, but I think you're right that if it's more involved it'd be dropped on the floor. Fix coming presently, thanks!

Previous code dropped publicPath on the floor when it was relative, which would have broken certain
corner cases.
@faceyspacey
Copy link
Owner

I like the URL route. As you mentioned, webpack-hot-middleware has issues with IE anyway, and polyfills are available:

webpack-contrib/webpack-hot-middleware#11
https://github.com/glenjamin/webpack-hot-middleware#use-on-browsers-without-eventsource
https://libraries.io/search?platforms=NPM&q=eventsource+polyfill

Otherwise, excellent work! Thanks for this.

@faceyspacey faceyspacey merged commit 36d0462 into faceyspacey:master Jul 17, 2017
@phyllisstein
Copy link
Contributor Author

De nada! Glad to give back. 💖

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.

3 participants