-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Make template.html and sw.js use publicPath #323
Make template.html and sw.js use publicPath #323
Conversation
There's already a PR for this. Just that it doesn't touch You can follow it on #315 |
4bd3de9
to
5f5f0f8
Compare
@@ -6,7 +6,7 @@ | |||
<meta name="viewport" content="width=device-width,initial-scale=1"> | |||
<meta name="mobile-web-app-capable" content="yes"> | |||
<meta name="apple-mobile-web-app-capable" content="yes"> | |||
<link rel="manifest" href="/manifest.json"> | |||
<link rel="manifest" href="<%= htmlWebpackPlugin.files.publicPath %>manifest.json"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it works right now -
<link rel="manifest" href="<%= __webpack_public_path__ %>manifest.json">
,
why can't we use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to stay away from __webpack_public_path__
where possible. I think it is generally bad practice to be reading from that as it is mutable and not so reliavle. For lib/entry.js
I used __webpack_public_path__
because it is the only available reference in the context.
I understand the inconsistency, but let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is mutable, and this how webpack itself proposes to use this variable if the publicPath
's value is not known at compilation time, so you can even set it in the entry point.
src/lib/entry.js
Outdated
@@ -5,7 +5,8 @@ if (process.env.NODE_ENV==='development') { | |||
require('preact/devtools'); | |||
} | |||
else if ('serviceWorker' in navigator && location.protocol === 'https:') { | |||
navigator.serviceWorker.register('/sw.js'); | |||
// eslint-disable-next-line no-undef | |||
navigator.serviceWorker.register(__webpack_public_path__ + 'sw.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@developit Please take a look at this and decide finally about the solution. I really need this feature in my project :-)
In my opinion it is a nice solution, what are your concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this, webpack will never change this var name and it keeps the resolution at runtime.
@lwakefield I don't understand, how the description of this PR correlates to the patch code? |
@rodush I'm not fully sure I understand why you think the PR description doesn't relate to the patch code? Please feel free to suggest edits! In the meantime, I have been using @lwakefield/preact-cli which has these changes. Maybe it can be of help, or you can create your own namespaced package? |
@lwakefield I prefer not to get locked to custom fork unless it is 100% unavoidable 😉 |
I understand. Perhaps, you can branch yourself and change the package when this (or another PR) is merged? |
@lwakefield here |
@lwakefield any update on this? Any plans about merging this PR? @developit ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the simplest approach to me. We'll need to consider processing manifest.json
and rewriting its URLs, or noting for users that they must manually set their homepage URL there to the public path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for now. Can optimize later on with the ideas we've all tossed around in this PR & in #315
* master: (166 commits) Use `publicPath` for SW & Manifest paths (#323) Purge `examples` dir (#393) Update Readme (#383) Add `preact list` command (#384) linters are dumb inject / replace strings in extracted contents adding flag to disable service worker Convert `css-loader` options to an Object (#349) remove `offline-plugin` (unused) remove `mkdirp` (unused) remove `less` dependencies fix linter move `htmlPlugin` to `render-html-plugin.js` install `css|postcss|style-loader` & regen `yarn.lock` remove `chunk.modules` deprecation notice rewrite server’s `webpack.config` without blocks rewrite client’s `webpack.config` without blocks install `webpack-merge` rewrite base `webpack.config` w/o blocks light refactor ... # Conflicts: # src/lib/webpack/webpack-base-config.js
Are there any updates on this? We need to use subdirectories for a couple of WebApps. I would love to use preact as we did lots of prototyping already. What is the latest idea to use subdirectories? I followed this tread and also the other related ones... not clear what is current state .... |
Did the recipe get added to the wiki?
I see a test for this so assume it's the accepted way: |
A recipe was never added, no. but indeed, that's the way to alter the public path if you need to. |
This PR aims to respect the use of
webpack.output.publicPath
and is intended as an alternative to #315.HTMLWebpackPlugin handles injecting the rest of the resources, so that you can set
output.publicPath
and the generatedindex.html
will reflect the change.The intent here is to prefer customizability through the already exposed
preact.config.js
instead of adding new config/env flags.I also propose that the following snippet be added to Config Recipes Wiki Page under the title "Specify the root url to serve from"