fix: check for http in baseUrl #993
Conversation
`path.posix.join` can't be used with URLs.
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/reyjhyr2i [update for fb90d10 canceled] |
There was a problem hiding this comment.
Oh nice! Good catch. Yea, we do this inside of Snowpack, but it looks like we forgot to inside of this plugin. @drwpow has been talking about a "common utils" library for plugin authors, a function that could do this and handle URLs automatically feels like it could fit in that bucket (we need it ourselves, internally)
1 small comment to resolve before merging, but overall LGTM
plugins/plugin-webpack/plugin.js
Outdated
| @@ -67,7 +68,9 @@ function emitHTMLFiles({ doms, jsEntries, stats, baseUrl, buildDirectory }) { | |||
|
|
|||
| for (const jsFile of jsFiles) { | |||
| const scriptEl = dom.window.document.createElement("script"); | |||
| scriptEl.src = path.posix.join(baseUrl, jsFile); | |||
| scriptEl.src = baseUrl.startsWith('http') | |||
There was a problem hiding this comment.
In other parts of the codebase, we've done if (url.parse(resolvedImportUrl).protocol) { which is slightly more general than just checking for http. Would love to see that here as well
There was a problem hiding this comment.
I had a look at how to implement url.parse(resolvedImportUrl).protocol to resolve the URL and as far as I can tell I have to import some stuff which needs some configuration where I have no clue how to get it inside the plugin context.
Mainly this part: https://github.com/pikapkg/snowpack/blob/master/snowpack/src/commands/dev.ts#L554.
I get the feeling that this would involve some bigger refactorings.
There was a problem hiding this comment.
I made the change that I was picturing, hopefully that's more clear / no big refactoring required at this point.
path.posix.joincan't be used with URLs.Changes
See #989.
Testing
I tested it locally and set-up a repl to replicate the problem:
https://repl.it/@stefanfrede/snowpack-webpack-jsdom#index.js