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

Allow users to specify homepage variable in package.json #315

Closed
wants to merge 8 commits into from

Conversation

ethanroday
Copy link
Contributor

  • Allow users to specify a homepage variable in package.json, referring to the public path under which the app is served in production. This may be a relative path or absolute URL, though only the pathname is relevant. The value of homepage is ignored in non-production environments.
  • Use homepage as Webpack's publicPath.
  • Expose process.env.HOMEPAGE so users can use the variable in their own code.
  • Prefix all asset paths (including the service worker path and mainfest path) with the public path.

Copy link
Member

@reznord reznord left a comment

Choose a reason for hiding this comment

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

There are a lot of indentation issues in this PR.

The tests are failing, can you please have fix them too. (specifically in build.snapshot.js)

/cc @lukeed for further thoughts on this.

@reznord reznord requested a review from lukeed August 15, 2017 14:09
@ethanroday
Copy link
Contributor Author

@reznord Fixed the indentation issues (sorry, tabs vs. spaces).

Regarding the failed checks, I can't think of why the service worker check would succeed on Node 6 and fail on Node 8. Unfortunately, I can't actually run the tests locally because I'm on a Windows machine and #262 prevents the preact serve tests from running. But I did just create, build, and http-server (with a proxy redirect through Fiddler to a domain with valid certificates) using Node 8 and the service worker registered just fine in Chrome.

I just looked at the pending checks for my latest commit, and the Node 6 check (which succeeded on the previous commit) seems to be failing all over the place. This is very confusing, since the only difference between those two commits was spacing.

@ethanroday
Copy link
Contributor Author

Meanwhile, the most recent Node 8 check is passing this time.
Any ideas why this might be happening? I just double-checked the diffs and it's just spacing.

@reznord
Copy link
Member

reznord commented Aug 15, 2017

I'm gonna test this locally in my PC and will let you know.

@reznord
Copy link
Member

reznord commented Aug 15, 2017

@ethanroday the tests pass perfectly in my local machine, tested it on node v6, v7, v8 too.

@ethanroday
Copy link
Contributor Author

Huh. @lukeed? @developit? Any ideas on why these tests would be succeeding locally but failing in Travis?

@lukeed
Copy link
Member

lukeed commented Aug 15, 2017

Admittedly, I know nothing about our tests 😞 Pinging the resident mastermind @rkostrzewski

@lukeed
Copy link
Member

lukeed commented Aug 18, 2017

Makes me wonder if we should start a "preact-cli" config key within package.json, instead of adding keys here & there.

@lwakefield
Copy link
Contributor

Perhaps this could also be solved setting publicPath directly. This would help keep custom configuration close together. Then this could be achieved like follows:

// preact.config.js
export default function (config, env, helpers) {
    const BASE_URL = env.production ? '/my-gh-pages-url' : ''
    config.output.publicPath = BASE_URL
}

@lukeed
Copy link
Member

lukeed commented Aug 18, 2017

@lwakefield Correct, I prefer that approach to be honest, but I'm not sure what others feel about it.

@lwakefield
Copy link
Contributor

Whatever the outcome, it may not be a bad idea to consolidate as much of the configuration as possible. Currently, it looks like there are 5 different ways to configure the preact-cli build:
browserlist, babel, webpack, pre-render and template.

@ethanroday
Copy link
Contributor Author

ethanroday commented Aug 18, 2017

@lwakefield That's actually what I've been doing as a workaround for now, but it leaves one problem unsolved: The path to the service worker is hardcoded as /sw.js and doesn't care what the public path is.

I see your point about there being too many entries for configuration - and this PR just adds another one (in the form of a homepage entry in package.json). I agree that only having to set the public path in preact.config.js would be vastly preferable, but somehow entry.js still needs to get the value of that variable so it can set the service worker path correctly (that's currently being achieved with process.env.HOMEPAGE). In your scenario, how does entry.js get access to the public path (short of requiring the user to define a specifically-named Webpack constant in their preact.config.js)?

@lwakefield
Copy link
Contributor

lwakefield commented Aug 18, 2017

@ethanroday, I agree - I ran into similar issues with /sw.js and I think this PR currently addresses this. Are we able to do the following to do away with process.env.HOMEPAGE:

// lib/entry.js
if (process.env.NODE_ENV === 'development') {
	require('preact/devtools');
} else if ('serviceWorker' in navigator && location.protocol === 'https:') {
	navigator.serviceWorker.register(__webpack_public_path__ + 'sw.js');
}

EDIT: see master...lwakefield:use-public-path-for-manifest - I can reopen #323 if it makes sense.

@ethanroday
Copy link
Contributor Author

@lwakefield A significantly more elegant solution! One note: in theory, the Webpack public path can be an absolute URL. In those cases, you would need to check to make sure it was the same domain as the current location before attempting to register the service worker.

With that change in place, though, I would be happy to abandon this in favor of a re-opened #323.

@lukeed
Copy link
Member

lukeed commented Aug 19, 2017

I agree. To make things simpler (re: /sw.js), let's just add the publicPath to the prerender options so that it's available to the template.

A quick-n-dirty fix is to add:

var baseUrl = '<%= htmlWebpackPlugin.options.publicPath %>';

/// entry.js
navigator.serviceWorker.register(baseUrl + 'sw.js');

Exposing/mounting baseUrl is safe since it's essentially equivalent to location.href on the first load.

@ethanroday
Copy link
Contributor Author

@lukeed, are you suggesting just adding baseUrl as a global variable in a script tag in template.html? Why is that preferable to @lwakefield's __webpack_public_path solution? Or am I misunderstanding you?

@ethanroday
Copy link
Contributor Author

@rkostrzewski, any thoughts on the failing tests?

@jojo05
Copy link

jojo05 commented Aug 26, 2017

Hi,
I did a build using "homepage": "pwa" and the preact.config.js from @lwakefield above and all URLs are prefixed properly except manifest.json.
We also need to figure out how to reconfigure the Routes (prefixing with pwa doesn't work either)

link rel="manifest" href="/manifest.json"
link rel="shortcut icon" href="/pwa/favicon.ico"
link href="/pwa/style.a1486.css" rel="stylesheet"

body
script defer="defer" src="/pwa/bundle.1405f.js"
script window.fetch||document.write('<script src="/pwa/polyfills.a668a.js"

@developit
Copy link
Member

We could use DefinePlugin to make PUBLIC_PATH available to script, then use that for the serviceworker URL.

@lwakefield
Copy link
Contributor

I would need to check that it works, but are you thinking of something like:

webpack.definePlugin('PUBLIC_PATH', JSON.stringify(__webpack_public_path__))

@jojo05
Copy link

jojo05 commented Aug 26, 2017

Any idea why the manifest.json is not properly prefixed?

Also, shouldn't both the dev an production show the real path ?
(I don't see a problem with dev showing 0.0.0.0:8080/pwa/)

The homepage option is really useful because any website can set up a PWA version of the site in a separate path. So, the Router integration is also key.

@rodush
Copy link

rodush commented Aug 28, 2017

Guys, any plans to solve this issue in the nearest couple days? I wouldn't want to create a fork for myself because of this... Thanks!

@reznord
Copy link
Member

reznord commented Aug 28, 2017

@rodush hey, for now you can just use preact.config.js which is suggested here: #315 (comment)

@rodush
Copy link

rodush commented Aug 28, 2017

@reznord It doesn't help, as the SW path is hardcoded in entry.js - /sw.js, this is the main problem.

@reznord
Copy link
Member

reznord commented Aug 28, 2017

Ah true, my bad!

@lwakefield
Copy link
Contributor

Hey, I reopened #323 as an alternative approach.

With respect to #315 (comment), after some experimenting, it is difficult to define PUBLIC_PATH without introducing another config/env variable. However, it is more or less a one-liner to exposePUBLIC_PATH in preact.config.js.

@lukeed
Copy link
Member

lukeed commented Oct 11, 2017

We decided to go with the path in #323 since it was simpler & doesn't involve any additional package.json reliance.

Thank you for your PR @ethanroday ❤️ It sparked some good ideas & conversations. We'll be revisiting those points in the near future, but for now, we're just going with the path of least resistance.

🙇

@lukeed lukeed closed this Oct 11, 2017
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.

7 participants