URL: Implement custom getQueryArgs, buildQueryString #20693
Conversation
aduth
added
[Status] In Progress
aduth
requested review from
ajitbohra,
gziolo,
nerrad,
ntwb and
talldan
as code owners
|
Size Change: -1.28 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
Also seen in the above #20693 (comment), there is a pretty nice drop. The results of |
aduth
force-pushed
the
remove/url-qs
branch
2 times, most recently
from
f6c0392 to
e5e0f60
Compare
|
Relevant comment at #20738 (comment) about the commit 8dfe648 here.
It's something I think we should want a consistent approach for in this module. Certain functions, we clearly want to expect a full URL (like Maybe the compromise is that for such functions, we specifically detect if the input is a root-relative URL and, if it is, construct e.g. let base;
if ( url.startsWith( '/' ) ) {
base = 'https://example.com';
}
const urlInstance = new URL( url, base ); |
There's some prior art for this approach by @sgomes. See https://github.com/Automattic/wp-calypso/blob/397d7592efea16154218b424602b15db64063361/client/lib/url/format.ts#L49 for an example. |
Calypso's I'm happy to accelerate any efforts in packaging it up if anyone's planning to reuse it! |
|
@sirreal @sgomes That's interesting, thanks for sharing! I could definitely see some value in some of this, such as detecting the "type" of a URL. I think at least for the immediate purposes here, the current implementation proposed here more-or-less matches how it's implemented in Calypso's For me, it's largely a question of documentation. Some of the functions of this module demand to operate only with a full URL ( |
|
I just realized this pull request was left with the "In Progress" label. As far as I can recall, this is complete and ready for review. |
gziolo
force-pushed
the
remove/url-qs
branch
from
e5e0f60 to
e3917d3
Compare
|
@sirreal, any idea why TypeScript errors:
|
gziolo
force-pushed
the
remove/url-qs
branch
from
37979b9 to
8e6066c
Compare
|
FYI @sgomes - removing |
|
There is one failing e2e test but I saw it before in other PRs and it doesn't seem to be relevant. |
This pull request seeks to refactor the
@wordpress/urlmodule to use a custom implementation of parsing and serializing query strings, removing theqsdependency in the process. The intent with these changes is to have a stronger contractual guarantee of alignment of behavior between@wordpress/urland corresponding PHP functions (build_query,parse_str). It's expected this may also result in a smaller overall bundle size, as we are not using the full extent of the feature set ofqs.Implementation Notes:
I had some early hopes to be able to use
URLSearchParamsfor what's implemented here, but it behaves quite differently than we expect from these functions, largely in two ways:+vs. as%20.URLSearchParamsuses%20(per URL Living Standard). PHP uses+(per RFC1866 form encoding).URLSearchParamshas limited support for "multiple" values (getAll,append), but these are formatted differently than they are in PHP (i.e. neither square brackets nor indices in the argument name). There's also no built-in support for "keyed" arguments.Testing Instructions:
Verify unit tests continue to pass: