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

Fix RegExp from navigateFallbackBlacklist (workbox) #7176

Merged
merged 1 commit into from Aug 8, 2019

Conversation

nuragic
Copy link
Contributor

@nuragic nuragic commented Jun 6, 2019

Hello,

This lovely RegExp was preventing my index.html to be served from ServiceWorker after an authentication callback; the request always hit the network…

Here is an example of this kind of "auth requests" callback:

https://example.com/PWA/callback?code=a55b99fbe42a3a7663b344020d2407047f4eb206aded15c300568ca4ef1219ce&scope=openid%20profile%20organizations%20api1%20offline_access&state=12c7c99a9b75476545161b45302e2cd&session_state=_p1MWLryvh08qxT-gmvucCIFVsWh8rIwo_MUBjhjvyY.5f6ee222da987db8c21aa4a35f93ada

The problem was that it was blacklisted by workbox because… it contains a . ! 😟

I believe it would be just safer to remove it… I hope this will prevent fellow devs opting in for SW wasting a lot of time like I did to found out! 😅

Thanks!

@Timer
Copy link
Contributor

Timer commented Jun 6, 2019

cc @jeffposnick

@jeffposnick
Copy link
Contributor

I did some digging into the history to refresh my memory. Here's the underlying issue that led to that configuration being used: #3008

Given that there's no easy way to enable/disable/override the default Workbox configuration options, I'd imagine that the conclusion about the most reasonable sett of defaults expressed in #3008 still apply, and just dropping the RegExp entirely is likely to cause issues.

If a modification to the RegExp so that it only matched on . characters before, rather than after, a ? character is sufficient, then that seems like a good compromise.

@nuragic
Copy link
Contributor Author

nuragic commented Jun 6, 2019

If a modification to the RegExp so that it only matched on . characters before, rather than after, a ? character is sufficient, then that seems like a good compromise.

Gotcha! I will just modify the regexp then. Thanks for the additional context.

@nuragic nuragic force-pushed the remove-workbox-problematic-regexp branch from a990103 to a51eff5 Compare June 7, 2019 08:01
@nuragic nuragic changed the title Remove problematic RegExp from navigateFallbackBlacklist (workbox) Fix RegExp from navigateFallbackBlacklist (workbox) Jun 7, 2019
@nuragic
Copy link
Contributor Author

nuragic commented Jun 7, 2019

Fixed. Let me know if you have any additional concerns.

Exclude URLs that contains a "?" character.
@nuragic nuragic force-pushed the remove-workbox-problematic-regexp branch from a51eff5 to 901636b Compare June 9, 2019 09:11
@jeffposnick
Copy link
Contributor

👍 from my point of view.

@nuragic
Copy link
Contributor Author

nuragic commented Jun 10, 2019

Cool, many thanks for the review!

@nuragic
Copy link
Contributor Author

nuragic commented Jun 18, 2019

👀 👋 I guess it's safe to merge?

@iansu iansu added this to the 3.0.2 milestone Jun 19, 2019
@iansu iansu merged commit efaee65 into facebook:master Aug 8, 2019
@iansu
Copy link
Contributor

iansu commented Aug 8, 2019

Thanks!

@nuragic
Copy link
Contributor Author

nuragic commented Aug 8, 2019

Cool thank you!

@lock lock bot locked and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants