Improve compatibility with uri-js#84
Conversation
|
Does this affect our benchmarks? |
|
Is there a perf implication on this? It should have. Can't we use a different function based on env? Node vs browser? |
|
Ok bench is not working, I will look into it later Here are the results on my machine, but Izmir is burning right now (37° celsius) so my results aren't 100% accurate — but still, I don't observe a significant drop anywhere: PR: fast-uri: parse domain x 1,963,346 ops/sec ±1.99% (94 runs sampled)
urijs: parse domain x 725,481 ops/sec ±0.38% (101 runs sampled)
WHATWG URL: parse domain x 4,164,643 ops/sec ±0.16% (100 runs sampled)
fast-uri: parse IPv4 x 2,869,202 ops/sec ±0.20% (101 runs sampled)
urijs: parse IPv4 x 588,070 ops/sec ±0.39% (99 runs sampled)
fast-uri: parse IPv6 x 1,309,017 ops/sec ±0.61% (101 runs sampled)
urijs: parse IPv6 x 422,369 ops/sec ±0.98% (98 runs sampled)
fast-uri: parse URN x 3,504,515 ops/sec ±1.26% (96 runs sampled)
urijs: parse URN x 1,823,102 ops/sec ±0.51% (96 runs sampled)
WHATWG URL: parse URN x 4,845,320 ops/sec ±0.43% (100 runs sampled)
fast-uri: parse URN uuid x 2,305,687 ops/sec ±0.52% (96 runs sampled)
urijs: parse URN uuid x 1,287,622 ops/sec ±0.59% (97 runs sampled)
fast-uri: serialize uri x 1,768,425 ops/sec ±0.70% (90 runs sampled)
urijs: serialize uri x 577,715 ops/sec ±3.03% (97 runs sampled)
fast-uri: serialize IPv6 x 544,162 ops/sec ±0.86% (93 runs sampled)
urijs: serialize IPv6 x 383,924 ops/sec ±0.51% (95 runs sampled)
fast-uri: serialize ws x 1,783,879 ops/sec ±0.78% (95 runs sampled)
urijs: serialize ws x 557,360 ops/sec ±0.63% (97 runs sampled)
fast-uri: resolve x 496,207 ops/sec ±1.09% (92 runs sampled)
urijs: resolve x 322,312 ops/sec ±0.89% (94 runs sampled)Main: fast-uri: parse domain x 1,935,556 ops/sec ±0.60% (96 runs sampled)
urijs: parse domain x 716,592 ops/sec ±0.57% (97 runs sampled)
WHATWG URL: parse domain x 4,050,297 ops/sec ±0.91% (97 runs sampled)
fast-uri: parse IPv4 x 2,752,662 ops/sec ±0.41% (97 runs sampled)
urijs: parse IPv4 x 594,033 ops/sec ±0.57% (96 runs sampled)
fast-uri: parse IPv6 x 1,314,395 ops/sec ±0.47% (97 runs sampled)
urijs: parse IPv6 x 399,563 ops/sec ±0.75% (97 runs sampled)
fast-uri: parse URN x 3,378,312 ops/sec ±0.75% (96 runs sampled)
urijs: parse URN x 1,716,716 ops/sec ±1.06% (93 runs sampled)
WHATWG URL: parse URN x 4,666,504 ops/sec ±0.67% (97 runs sampled)
fast-uri: parse URN uuid x 2,283,147 ops/sec ±0.83% (98 runs sampled)
urijs: parse URN uuid x 1,227,278 ops/sec ±0.89% (93 runs sampled)
fast-uri: serialize uri x 1,264,827 ops/sec ±0.71% (91 runs sampled)
urijs: serialize uri x 556,325 ops/sec ±0.98% (91 runs sampled)
fast-uri: serialize IPv6 x 484,639 ops/sec ±0.99% (91 runs sampled)
urijs: serialize IPv6 x 351,026 ops/sec ±1.50% (93 runs sampled)
fast-uri: serialize ws x 1,296,084 ops/sec ±1.59% (92 runs sampled)
urijs: serialize ws x 520,719 ops/sec ±0.97% (87 runs sampled)
fast-uri: resolve x 407,145 ops/sec ±1.02% (88 runs sampled)
urijs: resolve x 304,759 ops/sec ±0.62% (90 runs sampled) |
| } | ||
| if (parsed.path !== undefined && parsed.path.length) { | ||
| parsed.path = encodeURI(parsed.path) | ||
| parsed.path = escape(unescape(parsed.path)) |
There was a problem hiding this comment.
Could you elaborate this change?
These functions are deprecated:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI?retiredLocale=it
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape
> escape(unescape('шеллы'))
'%u0448%u0435%u043B%u043B%u044B'
> encodeURI('шеллы')
'%D1%88%D0%B5%D0%BB%D0%BB%D1%8B'
There was a problem hiding this comment.
Hey,
So I want to start by saying that I fully agree with you in that deprecated functions should be avoided in general, but they have different behavior as you pointed out and this change is to achieve url-js compatibility (one of the goals of this library) — you can see that escape and unescape have already been used many times in this file:
Lines 265 to 274 in bf03d44
In fact, if I recall correctly, someone tried to change these functions to encode/decodeURI, and maybe the tests didn't catch the behavior change for path
Having said all that, I don't think these functions will ever be removed from the Web
Because we have this in the Todos/Goals, I feel it should be bugfix/patch release Does anyone object? Cc @fastify/libraries @fastify/plugins |
|
Thanks all, I will prepare a new release of AJV with fast-uri. Let's try and fix forward if anything else comes up. |
|
Absolutely! |
this test must pass if we want to merge fast-uri into ajv
I'm about to fix it, hence the PR