w3c / ServiceWorker

Service Workers
https://w3c.github.io/ServiceWorker/
Other
3.63k stars 313 forks source link

Consider deprecating the ability for scope to match query parameters #1469

Open mgiuca opened 5 years ago

mgiuca commented 5 years ago

This has come up in regards to @wanderview 's scope pattern matching proposal; in particular on issue #4. I've come to think that the current ability to match the query / search part of a URL in the service worker scope is totally broken; it will almost never produce the desired results and there isn't a way to make it useful. So I would like us to consider deprecating or removing it.

The scope matching algorithm performs an exact prefix match to determine which URLs are in scope. It is never explicitly stated in the SW spec, but this applies to the query part of the scope as well. Even when applied to the path, this is less than desirable, but when applied to the query, it makes almost no sense. The general form of a query string in a URL is that it represents an unordered collection of key=value pairs, delimited by &s.

As an example, if we imagine a scope /foo?bar=baz, the author probably intended this to match URLs with a key "bar" having the value "baz", but there are two mistakes here:

There is no way to fix either of these issues with the current scope syntax. For example, you could try adding a '&' to the end of the scope, which would limit it to an exact match, but then it would require that there be another query parameter after "bar". And there is no way to make the query parameter matching order-independent.

Supporting this in the future represents an ongoing headache:

Given this, I can't imagine anybody is using this feature correctly. I would like to propose deprecation of this feature (e.g., a large red box in the spec, and recommending that user agents display a warning in the console if a '?' appears in the scope during registration), and perhaps removal if its usage is sufficiently low. https://github.com/wanderview/service-worker-scope-pattern-matching proposes a way to match query parameters correctly.

jakearchibald commented 5 years ago

The scope is a prefix, so it seems pretty obvious that order of things in the query string matters.

I don't think we can just add pattern matching to regular scope strings (and other places we use URLs). * crops up in most patterns, and it's also valid in a URL.

If someone deliberately puts ?foo in a scope string, it seems weird to just ignore it. Shouldn't it do something, or throw?

mgiuca commented 5 years ago

The scope is a prefix, so it seems pretty obvious that order of things in the query string matters.

Yes, but I don't think it's particularly obvious that the scope is a prefix match (if you just learn by example, as opposed to reading the spec, I think it's fairly unintuitive both a) that the path is prefix-based as opposed to hierarchical; see #1272, and b) that the query is prefix-based as opposed to matching individual key=value pairs).

Up until now, I hadn't really thought about the query part, and I was surprised to realise that it behaves this way (even though I had previously read the spec that it was a prefix match, I didn't put 2 and 2 together to realise that this imposes a strict ordering on the query part of a URL). As I said above, I don't think there's really any way to use this feature correctly.

I don't think we can just add pattern matching to regular scope strings (and other places we use URLs). * crops up in most patterns, and it's also valid in a URL.

I think @wanderview is doing a good job figuring out how to add more flexible scope matching (on wanderview/service-worker-scope-pattern-matching) so I'm not proposing any new syntax here. I just think it would be good to remove this mis-feature.

If someone deliberately puts ?foo in a scope string, it seems weird to just ignore it. Shouldn't it do something, or throw?

It should probably log a warning. Even if we don't ignore it, I think we should still advise implementations to log a warning because of the surprising behaviour.

Note that the Manifest spec does just ignore the query.

wanderview commented 5 years ago

If we want to remove this feature I think we would just strip the search params from the navigation url before performing the prefix match. We would probably need to collect data to see how much this is being used in the wild.