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

should URLPattern match a trailing "/" by default like path-to-regexp? #14

Closed
wanderview opened this issue Oct 9, 2020 · 13 comments
Closed
Labels

Comments

@wanderview
Copy link
Member

During the call in w3c/ServiceWorker#1535 we discussed the optional prefix behavior. For example:

new URLPattern({ pathname: '/foo/:bar?' });

Would match:

  • /foo
  • /foo/xyz

But would not match:

  • /foo/
  • /foobar

The main point of the feature is to avoid matching the /foobar here, but not matching /foo/ may be a problem for sites that normalize URLs with a trailing slash.

This discrepancy could be because the proposal is currently based on "strict" path-to-regexp mode instead of "default" mode. In "default" mode a trailing "/" is always permitted.

This issue is to figure out how to handle this. Should we switch the proposal to use default mode which is what most developers use? Should we do something different for optional prefix behavior?

@blakeembrey
Copy link

blakeembrey commented Oct 15, 2020

To clarify, in path-to-regexp there is two issues:

  1. Non-strict mode (default) allows an optional trailing / character (regardless of what you write, it's basically like adding {/}? to end end)
  2. Patterns like :foo automagically "consume" the prefix and make it part of the match

I think generally the web expects 1 somewhat, but 2 is likely a historical mistake to support. However, the issue with 1 and the current API proposal is that it makes explicit matching impossible so it may be worth using only "strict" mode.

It could be possible to force people to be explicit with the trailing slash so they can mirror their server behavior (maybe with /?). Examples of where a trailing slash is optional can be seen on pretty much every static site router and even on GitHub (e.g. https://github.com/WICG/ and https://github.com/WICG both resolve without redirecting).

@wanderview
Copy link
Member Author

In #22 @blakeembrey suggests a possible alternative to the magic optional prefix behavior. Instead of writing something like:

/foo/:bar?

You would instead use a new grouping mechanism to scope what the modifier applies to:

/foo{/:bar}?

Since the / is in the {} scope then the ? modifier applies to it. If you don't want to make the / optional then you would instead write /foo/{:bar}? or just /foo/:bar.

I'm not sure this would allow us to specify a valid trailing "/", though.

@jakearchibald would this make any of your use cases easier?

@wanderview
Copy link
Member Author

Updating the issue title to reflect that this is mainly about the path-to-regexp non-strict default mode.

I think for service worker scopes we definitely need strict mode. (We don't accept an optional trailing delimiter if it wasn't explicitly included in the pattern.)

For URLPattern js use I'm unsure what to do.

  1. We can match path-to-regexp and default to non-strict mode.
  2. Or we could align more towards the platform conventions and default to string mode.

In either case it seems we probably need an option to URLPattern to override the default.

I think @domenic was in favor of aligning with the platform here.

@wanderview wanderview changed the title should "optional prefix" behavior match ending "/" should URLPattern match a trailing "/" by default like path-to-regexp? Nov 18, 2020
@wanderview
Copy link
Member Author

Just an observation. In regards to the original post in this issue, this example:

new URLPattern({ pathname: '/foo/:bar?' });

Only doesn't match /foo/ because the expression basically says:

  1. Require /:bar to match where :bar must be one or more non-slash characters.
  2. Or allow /:bar to be completely missing, meaning there is no slash either.

If we used a custom regexp to not require at least one character in :bar:

new URLPattern({ pathname: '/foo/:bar([^\/]*)?' });

Then it does match /foo/.

@annevk
Copy link
Member

annevk commented Nov 30, 2020

FWIW, it seems reasonable to make a trailing slash optional and match either way, but requiring a trailing slash and requiring it not to be there should also be possible.

@wanderview
Copy link
Member Author

Yea. I think its easier, though, to default to strict mode then since you can add an optional trailing slash in your pattern directly.

In the opposite case where strict is disabled by default, though, there is no way to write a pattern that says "disable optional trailing slash behavior". Instead it would require a "enable strict mode" option outside of the pattern.

@annevk
Copy link
Member

annevk commented Dec 1, 2020

(I don't think I'm a good help with the exact syntax, regular expressions always confuse me.) I wonder if we should write down some scenarios to see if the syntax can handle them. E.g., you might want to match

  • /foo
  • /foo/
  • /foo/xyz

but not

  • /fooxyz (or /fooxyz/)
  • /foo/xyz/

(It might also be interesting to think about this from the perspective of the model of a URL, where path is a list of strings. So you could think about how many items you would want to have in the list, regardless of whether they are the empty string or not.)

@wanderview
Copy link
Member Author

Do accomplish the above in strict mode you would have to use a custom regexp group like:

  • /foo/([^/]*)?

You could make this a named group with the custom regexp like:

  • /foo/:bar([^/]*)?

I don't think you can accomplish the above example in non-strict mode since it will always match a trailing slash which is one of your excluded cases. You could meet all the other constraints in non-strict mode with /foo/:bar? without using a custom regexp group.

I think at this point I am planning to make a go at using strict mode as the default without an option to disable it. If we get feedback that its necessary we can add the option (or something else more extreme if the feedback is very loud).

@wanderview
Copy link
Member Author

I also realized if a developer wants non-strict behavior they can easily achieve it by appending {/}? to the end of their pathname pattern. This seems quite short and explicit, so I will probably steer folks towards this instead of adding an option to disable strict mode.

@wanderview
Copy link
Member Author

The only tricky thing would be if many developers want to use a trailing {/}? non-strict pattern with web platform APIs like service workers. My restrictions for service workers today would not permit it. We could try to support it, though.

@wanderview
Copy link
Member Author

Note, I opened a github "discussion" to collect community feedback on this issue. See #38.

@wanderview
Copy link
Member Author

I think we've decided to go with strict mode.

@wanderview
Copy link
Member Author

This has been codified in the spec.

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

No branches or pull requests

3 participants