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

Make template.html and sw.js use publicPath #323

Merged
merged 4 commits into from
Oct 11, 2017

Conversation

lwakefield
Copy link
Contributor

@lwakefield lwakefield commented Aug 18, 2017

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 generated index.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"

// preact.config.js
import { defineConstants } from '@webpack-blocks/webpack2';

export default function (config, env, helpers) {
  const PUBLIC_PATH = env.production ? '/my-gh-pages-url' : ''
  config.output.publicPath = PUBLIC_PATH
  config.plugins.push(defineConstants({ PUBLIC_PATH }))
}

@reznord
Copy link
Member

reznord commented Aug 18, 2017

There's already a PR for this. Just that it doesn't touch preact.config.js.

You can follow it on #315

@lwakefield
Copy link
Contributor Author

Ah, my mistake @reznord! I'll close this in favour of #315

@lwakefield lwakefield closed this Aug 18, 2017
@lwakefield lwakefield changed the title Make template.html use publicPath for manifest Make template.html and sw.js use publicPath Aug 31, 2017
@lwakefield lwakefield reopened this Aug 31, 2017
@lwakefield lwakefield force-pushed the use-public-path-for-manifest branch from 4bd3de9 to 5f5f0f8 Compare September 6, 2017 12:43
@@ -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">
Copy link

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?

Copy link
Contributor Author

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!

Copy link

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');
Copy link

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?

Copy link
Member

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.

@rodush
Copy link

rodush commented Sep 12, 2017

@lwakefield I don't understand, how the description of this PR correlates to the patch code?

@lwakefield
Copy link
Contributor Author

@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?

@rodush
Copy link

rodush commented Sep 14, 2017

@lwakefield I prefer not to get locked to custom fork unless it is 100% unavoidable 😉
Since this is quite young project, would be very nice to have this important option out of the box, that is why I'm hoping that the owner (@developit) will make a final decision.

@lwakefield
Copy link
Contributor Author

I understand. Perhaps, you can branch yourself and change the package when this (or another PR) is merged?

@rodush
Copy link

rodush commented Sep 14, 2017

@lwakefield here const PUBLIC_PATH = env.production ? '/my-gh-pages-url' : '' I would suggest '/' as default, otherwise you risk to have a relative urls ;-) that is what I was fighting with with an Angular build to be served from subfolder

@rodush
Copy link

rodush commented Sep 28, 2017

@lwakefield any update on this? Any plans about merging this PR? @developit ?

@lwakefield
Copy link
Contributor Author

@rodush, still waiting on review of either this or #315.

Copy link
Member

@developit developit left a 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.

@developit developit requested a review from lukeed September 28, 2017 19:34
Copy link
Member

@lukeed lukeed left a 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

@lukeed lukeed merged commit 52e9843 into preactjs:master Oct 11, 2017
developit added a commit that referenced this pull request Oct 11, 2017
* 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
@tschnoelzer
Copy link

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 ....

@imaitland
Copy link

Did the recipe get added to the wiki?

I also propose that the following snippet be added to Config Recipes Wiki Page under the title "Specify the root url to serve from"

I see a test for this so assume it's the accepted way:
https://github.com/preactjs/preact-cli/blob/7d33cd1380b5dc7d4b4b970eceabfc8f126da1af/packages/cli/tests/subjects/public-path/preact.config.js

@rschristian
Copy link
Member

A recipe was never added, no. but indeed, that's the way to alter the public path if you need to.

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.

8 participants