feat: disable semicolon separator by default #336
Conversation
There was a problem hiding this comment.
This is really weird, but it looks like it's correct. Can you provide a bench results?
@mcollina This is a major for find-my-way.
| @@ -88,6 +88,7 @@ function Router (opts) { | |||
| this.maxParamLength = opts.maxParamLength || 100 | |||
| this.allowUnsafeRegex = opts.allowUnsafeRegex || false | |||
| this.constrainer = new Constrainer(opts.constraints) | |||
| this.useSemicolonDelimiter = opts.useSemicolonDelimiter || false | |||
There was a problem hiding this comment.
Don't forget to set it true in the Fastify.
There was a problem hiding this comment.
Ideally this option should be disabled in Fastify as well, but I don't know the best way to do it: leave the default false here and open issue/PR to update dependencies in Fastify, or make it true here and send PR to Fastify with the option disabled
There was a problem hiding this comment.
I would leave the false here, because it's a correct behavior and we can release a new major of fmw at any moment. In current version of Fastify it should be set to true by default, because otherwise it's a breaking change.
About changing it to false in the fastify in a future release. It's better to open an issue about this in fastify, but IMHO it never would be set to true by defauld in the Fastify, because it will break too many apps. It just doesn't worth it even if it's true according to the spec.
| // Thus, we need to split on `;` as well as `?` and `#`. | ||
| } else if (charCode === 63 || charCode === 59 || charCode === 35) { | ||
| // Thus, we need to split on `;` as well as `?` and `#` if the useSemicolonDelimiter option is enabled. | ||
| } else if (charCode === 63 || charCode === 35 || (useSemicolonDelimiter && charCode === 59)) { | ||
| querystring = path.slice(i + 1) |
There was a problem hiding this comment.
If there is any perf difference (I doubt), you can move the useSemicolonDelimiteradditional check under the charCode checking if. That would remove it from a hot path.
There was a problem hiding this comment.
Yes, given that the ; symbol is rare, that's correct. I fixed
I made a simple example in the sandbox btw, it works the same way for the |
|
@levensta did you run the benchmarks by any chance? |
I run screenshot
|
|
@mcollina I've updated the README. But I am still confused by the fact that there is still a |
|
I'm lost. can you make an example? |
In this example, you can see that both |
|
That does not look correct, what does the rfc say? |
|
Sorry for the long response time. The RFC explicitly says that the "#" character is used as the beginning of a separate segment called a fragment
The problem with the current code is that it truncates the path to the delimiter character, but does not check which one is a query string and which one is a fragment and writes everything into one A fix could look like this Should this behaviour be hidden behind the same parameter as |
|
It is correct that # should split. It is the "anker" in browser url, where you jump to the specific position. The correct behavior is to ignore everything after a #. So in conclusion: everything after the first occurence of |
This PR comes from the discussion in the issue (fastify/fastify#5050). I've added a new parameter that allows the
;character to be used to separate path and query as before, but its default value isfalseto be fully compliant with the RFC