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

Audit claims start_url of /?utm_ is not cached when navigateFallback exists #2688

Open
keanulee opened this Issue Jul 13, 2017 · 15 comments

Comments

Projects
None yet
8 participants
@keanulee
Member

keanulee commented Jul 13, 2017

In my manifest I specify start_url as something with utm_ params (as suggested by https://developers.google.com/web/fundamentals/engage-and-retain/web-app-manifest/#set_a_start_url):

"start_url": "/?utm_source=homescreen"

My SW precaches index.html and serves it as navigateFallback (as generated by sw-precache), but Lighthouse doesn't seem to recognize this and fails the audit with "Failures: Manifest start_url is not cached by a Service Worker."

@patrickhulce patrickhulce added the bug label Jul 13, 2017

@patrickhulce

This comment has been minimized.

Show comment
Hide comment
@patrickhulce

patrickhulce Jul 13, 2017

Member

Ah this is a good use case for us to cover thanks for the report @keanulee!

More information on navigateFallback

Member

patrickhulce commented Jul 13, 2017

Ah this is a good use case for us to cover thanks for the report @keanulee!

More information on navigateFallback

@brendankenny

This comment has been minimized.

Show comment
Hide comment
@brendankenny

brendankenny Jul 13, 2017

Member

@keanulee do you have a URL to share for easy testing? (otherwise it's not that difficult to set something up)

The issue is that we use the value of start_url to find the network record of requesting the page at start_url. If we can use a different method of identifying the record (e.g. moving the request out of pass() and do custom tracking of the fetch instead of relying on networkRecords), then just a status code of 200 and _fetchedViaServiceWorker should be a sufficient to correctly pass this case.

The tradeoff is that Lighthouse would more easily give false passes to sites that don't actually cache their start_url page, but have an offline fallback page set up. I'm not sure if there's a way out of the tension between these two.

@jeffposnick for funsies.

Member

brendankenny commented Jul 13, 2017

@keanulee do you have a URL to share for easy testing? (otherwise it's not that difficult to set something up)

The issue is that we use the value of start_url to find the network record of requesting the page at start_url. If we can use a different method of identifying the record (e.g. moving the request out of pass() and do custom tracking of the fetch instead of relying on networkRecords), then just a status code of 200 and _fetchedViaServiceWorker should be a sufficient to correctly pass this case.

The tradeoff is that Lighthouse would more easily give false passes to sites that don't actually cache their start_url page, but have an offline fallback page set up. I'm not sure if there's a way out of the tension between these two.

@jeffposnick for funsies.

@keanulee

This comment has been minimized.

Show comment
Hide comment
@keanulee

keanulee Jul 13, 2017

Member

Unfortunately I don't have a test URL handy (since I changed the start_url of what I'm working on back to / to pass this test). RE that tradeoff though, IMO as long as the app developer provides an offline fallback, it means that they put some thought into the offline use case so I think they should pass anyways.

Member

keanulee commented Jul 13, 2017

Unfortunately I don't have a test URL handy (since I changed the start_url of what I'm working on back to / to pass this test). RE that tradeoff though, IMO as long as the app developer provides an offline fallback, it means that they put some thought into the offline use case so I think they should pass anyways.

@patrickhulce

This comment has been minimized.

Show comment
Hide comment
@patrickhulce

patrickhulce Jul 13, 2017

Member

I agree. I'm in favor of just requiring that https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/start-url.js#L43 comes back without failure. If we can request your startUrl in the context of the page and get a successful response, then why does it matter if they redirected or supplied a fallback?

Member

patrickhulce commented Jul 13, 2017

I agree. I'm in favor of just requiring that https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/start-url.js#L43 comes back without failure. If we can request your startUrl in the context of the page and get a successful response, then why does it matter if they redirected or supplied a fallback?

@brendankenny

This comment has been minimized.

Show comment
Hide comment
@brendankenny

brendankenny Jul 13, 2017

Member

Yeah, I think I agree, I'm just thinking about the other side of things: for anyone with a navigateFallback (or equivalent), the start_url check on failure becomes a navigateFallback check, and that may or may not be on purpose. And it's a difficult issue to catch if Lighthouse is your only PWA CI tool.

Maybe we can add a future custom audit that allows stronger assertions about what the start_url should actually load to pick up the slack.

Member

brendankenny commented Jul 13, 2017

Yeah, I think I agree, I'm just thinking about the other side of things: for anyone with a navigateFallback (or equivalent), the start_url check on failure becomes a navigateFallback check, and that may or may not be on purpose. And it's a difficult issue to catch if Lighthouse is your only PWA CI tool.

Maybe we can add a future custom audit that allows stronger assertions about what the start_url should actually load to pick up the slack.

@brendankenny

This comment has been minimized.

Show comment
Hide comment
@brendankenny

brendankenny Jul 14, 2017

Member

hmm, I just did a quick test of this, and a navigateFallback (with a /?utm_source=homescreen fragment) should actually already be passing.

The network record has a url of what was requested, not what was returned, so as long as the SW is returning something when fetching start_url, the audit should match the network record and pass.

I've either messed up the test or there's something else causing the failure, so we'll need to come up with a better repro of the problem.

Member

brendankenny commented Jul 14, 2017

hmm, I just did a quick test of this, and a navigateFallback (with a /?utm_source=homescreen fragment) should actually already be passing.

The network record has a url of what was requested, not what was returned, so as long as the SW is returning something when fetching start_url, the audit should match the network record and pass.

I've either messed up the test or there's something else causing the failure, so we'll need to come up with a better repro of the problem.

@jeffposnick

This comment has been minimized.

Show comment
Hide comment
@jeffposnick

jeffposnick Jul 14, 2017

Contributor

I see a problem with the start-url gatherer: it performs a generic fetch(), like you'd make when requesting a subresource, but the logic for navigateFallback only kicks in when event.request.mode === 'navigate'. This isn't specific to sw-precache; a lot of the other offline-first libraries have fetch handlers which check for navigations and take specific action.

You can't programmatically construct (see item 17) a Request object that has mode === 'navigate'.

This has come up before: see #578

Contributor

jeffposnick commented Jul 14, 2017

I see a problem with the start-url gatherer: it performs a generic fetch(), like you'd make when requesting a subresource, but the logic for navigateFallback only kicks in when event.request.mode === 'navigate'. This isn't specific to sw-precache; a lot of the other offline-first libraries have fetch handlers which check for navigations and take specific action.

You can't programmatically construct (see item 17) a Request object that has mode === 'navigate'.

This has come up before: see #578

@patrickhulce

This comment has been minimized.

Show comment
Hide comment
@patrickhulce

patrickhulce Jul 14, 2017

Member

Ah thanks for the insight @jeffposnick. Looks like we'll have to end up saving the startUrl for the offline pass to use then after all

Member

patrickhulce commented Jul 14, 2017

Ah thanks for the insight @jeffposnick. Looks like we'll have to end up saving the startUrl for the offline pass to use then after all

@brendankenny

This comment has been minimized.

Show comment
Hide comment
@brendankenny

brendankenny Jul 14, 2017

Member

Thanks @jeffposnick. Piece of institutional memory we lost between #578 and now :)

For testing, we should maybe add a fallback like that to the SW of the offline-ready.html smoke test. That way we won't forget again.

For the audit, I think we still don't want to navigate to start_url if we can avoid it. With the recent talk of connecting Lighthouse directly to the service worker as well as the test page, I wonder if we should return to the super-old idea of generating a manual fetch event in the SW's context and seeing how it responds. I vaguely recall initial tests showing that the SW couldn't tell the difference between real and devtools-generated fetch events. The issue at the time was all the target, targetmanager, etc etc that needed to be implemented, but that's sort of already in progress for different reasons now :)

Member

brendankenny commented Jul 14, 2017

Thanks @jeffposnick. Piece of institutional memory we lost between #578 and now :)

For testing, we should maybe add a fallback like that to the SW of the offline-ready.html smoke test. That way we won't forget again.

For the audit, I think we still don't want to navigate to start_url if we can avoid it. With the recent talk of connecting Lighthouse directly to the service worker as well as the test page, I wonder if we should return to the super-old idea of generating a manual fetch event in the SW's context and seeing how it responds. I vaguely recall initial tests showing that the SW couldn't tell the difference between real and devtools-generated fetch events. The issue at the time was all the target, targetmanager, etc etc that needed to be implemented, but that's sort of already in progress for different reasons now :)

@neavilag

This comment has been minimized.

Show comment
Hide comment
@neavilag

neavilag Jul 24, 2017

Hi My PWA app.climadiario.com fails this audit test of LightHouse with "Failures: Manifest start_url is not cached by a Service Worker." but actually the app is prompting users to ADD to homescreen.

This is what I have in manifest.json. Is this a bug ?

 "orientation": "portrait",
  "Scope": "/",
  "start_url": "/index.html",

neavilag commented Jul 24, 2017

Hi My PWA app.climadiario.com fails this audit test of LightHouse with "Failures: Manifest start_url is not cached by a Service Worker." but actually the app is prompting users to ADD to homescreen.

This is what I have in manifest.json. Is this a bug ?

 "orientation": "portrait",
  "Scope": "/",
  "start_url": "/index.html",
@midzer

This comment has been minimized.

Show comment
Hide comment
@midzer

midzer Jul 24, 2017

Contributor

@neavilag Hi. I have "start_url": "/" for my root page with pretty URLs and not receiving an error from lighthouse.

Contributor

midzer commented Jul 24, 2017

@neavilag Hi. I have "start_url": "/" for my root page with pretty URLs and not receiving an error from lighthouse.

@abdonrd

This comment has been minimized.

Show comment
Hide comment
@abdonrd

abdonrd Oct 13, 2017

Any news about this?

It prevents getting 100 on PWA section. 😕

abdonrd commented Oct 13, 2017

Any news about this?

It prevents getting 100 on PWA section. 😕

@patrickhulce

This comment has been minimized.

Show comment
Hide comment
@patrickhulce

patrickhulce Oct 13, 2017

Member

@abdonrd it's been prioritized and will be unblocked when the work for #709 has been completed.

Member

patrickhulce commented Oct 13, 2017

@abdonrd it's been prioritized and will be unblocked when the work for #709 has been completed.

@abdonrd

This comment has been minimized.

Show comment
Hide comment
@abdonrd

abdonrd Oct 13, 2017

Sounds great, thanks @patrickhulce!

abdonrd commented Oct 13, 2017

Sounds great, thanks @patrickhulce!

@ShashankJainOYO

This comment has been minimized.

Show comment
Hide comment
@ShashankJainOYO

ShashankJainOYO Nov 16, 2017

Looks like 709 is still open. What's the plan on this one?

ShashankJainOYO commented Nov 16, 2017

Looks like 709 is still open. What's the plan on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment