Core: Migrate from `qs` to `picoquery` #28315

43081j · 2024-06-23T00:11:48Z

Moves the manager-api, preview-api and ui/manager to use picoquery instead of qs - a much smaller and faster alternative.

Note that we still have qs in our overall monorepo tree, but that can be tackled in a follow-up PR.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests (existing tests should suffice)
integration tests
end-to-end tests

Manual testing

Existing stories and tests should continue to function correctly.

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

nx-cloud · 2024-06-23T00:24:40Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4468a15. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

Moves the `manager-api`, `preview-api` and `ui/amanger` to use `picoquery` instead of `qs` - a much smaller and faster alternative. Note that we still have `qs` in our overall monorepo tree, but that can be tackled in a follow-up PR.

shilman · 2024-06-23T14:28:33Z

Looks like there are a few extra qs references that are causing linting to fail!

43081j · 2024-06-23T14:38:16Z

whoops my mistake! i forgot to run --task check i think

looks like you got it building though. let me know if you need any changes or help

shilman · 2024-06-23T14:40:00Z

I swapped one of them over, but didn't go the extra distance to do the other two, which are a little more involved 🙈

43081j · 2024-06-23T14:42:26Z

no worries, its probably the one i left out on purpose

happy to tackle that in its own PR as long as the rest of the packages build ok

sure means we temporarily have both in the dependency tree but its small enough we'll be ok i think, and i can probably sort it fairly soon

Uses the `foo.bar[0]` syntax picoquery provides, rather than dot-syntax (`foo.bar.0`).

43081j · 2024-06-24T17:28:43Z

note there's still one usage left i need to tackle

i have failing tests locally for it but will get to it at some point 👍

Moves the preview api to use picoquery. Note that tests will not pass yet.

ghengeveld

I'm happy to move from qs to picoquery, but only if it involves no functional changes. Unfortunately, it seems picoquery urlencodes args, which aren't currently encoded. This would be a breaking change and make the URLs look really ugly, so I'm opposed to changing that behavior.

code/lib/preview-api/src/modules/preview-web/UrlStore.ts

code/lib/router/src/utils.test.ts

ghengeveld · 2024-06-25T12:37:15Z

code/lib/router/src/utils.ts

+export const queryFromLocation = (location: Partial<Location>) =>
+  queryFromString(location.search ? location.search.slice(1) : '');


Suggested change

export const queryFromLocation = (location: Partial<Location>) =>

queryFromString(location.search ? location.search.slice(1) : '');

export const queryFromLocation = (location: Partial<Location>) =>

queryFromString(location.search.slice(1));

ghengeveld · 2024-06-25T12:41:59Z

code/ui/manager/src/components/preview/utils/stringifyQueryParams.tsx


 export const stringifyQueryParams = (queryParams: Record<string, string>) =>
-  qs.stringify(queryParams, { addQueryPrefix: true, encode: false }).replace(/^\?/, '&');


I think we need some unit tests to verify stringifyQueryParams doesn't change between the two implementations.

43081j · 2024-06-25T13:25:44Z

I'm happy to move from qs to picoquery, but only if it involves no functional changes. Unfortunately, it seems picoquery urlencodes args, which aren't currently encoded. This would be a breaking change and make the URLs look really ugly, so I'm opposed to changing that behavior.

this is true, however it is doing the right thing here while qs/storybook hasn't been

the URI spec defines it this way, and is why URLSearchParams also encodes them

so we're blocked here i guess. i understand the URLs looked nicer, but they're non-standard and its unlikely pq will ever support that (since it is trying to conform to the standard and be interchangeable with URLSearchParams in those cases)

the dependency tree & package size reduction is what i've been chipping away at for months now. it'd be a shame to back off from that for nice looking URLs, especially given qs is the last piece before we shed a bunch of weight (the last connection to a deep subtree of deps)

alternatively, we could have code inside storybook which literally does replace('%3B', '[') etc. i'm not a fan but at least it doesn't result in veering away from standards in PQ and can live here instead

ghengeveld · 2024-06-25T14:30:07Z

alternatively, we could have code inside storybook which literally does replace('%3B', '[') etc. i'm not a fan but at least it doesn't result in veering away from standards in PQ and can live here instead

That works for me. It's hacky but at least it's consistent. If being noncompliant turns out to be an issue then we can always change it in a major version release (in which case I'd opt for a different/custom array encoding to avoid %5B / %5D).

43081j · 2024-06-25T14:48:29Z

no worries, that works for me too

we already do some transforms on the parsed and stringified queries so it won't be much to add to that

Decodes certain chars we want to keep in our URIs (e.g. `[`).

43081j · 2024-06-25T16:49:05Z

we have some failures ill have to get to at some point another day 👍

basically what to do about invalid keys since pq will deal with them differently than qs did (for perf mostly)

e.g. a[b:val will likely result in {a: []}

ghengeveld · 2024-06-25T18:36:28Z

basically what to do about invalid keys since pq will deal with them differently than qs did (for perf mostly)

e.g. a[b:val will likely result in {a: []}

That’s okay. Invalid input doesn’t need to be parsed in any particular way. The most important thing is that data must deserialize back to what it was before serialization, to the extent that that’s possible.

43081j · 2024-06-28T16:57:11Z

i have a busy few days ahead and some travel coming up but ill get to this when i can 👍

will ping you when i manage to fix the remaining tests

43081j · 2024-07-28T09:36:25Z

just picking this up again @ghengeveld , any idea what this kind of stuff was trying to catch?

      expect(parseArgsParam('obj.foo[][a!b]:val;obj.foo.bar:val;obj.baz:val')).toStrictEqual({});

this syntax isn't valid anywhere really, obj.foo[][a!b]. qs does what it can with it, but its meant to be obj[foo][][a!b] or obj.foo[].a!b, etc. these inputs are pretty inconsistent, mixing dot-syntax and bracket-syntax

ultimately, its trying to test that nested keys are checked for validity. so maybe i can just update these to be more sensible inputs?

edit: did what i could, think i managed to sort it out. those tests could probably do with a rewrite given how specific they are to qs behaviour (but in another PR)

JReinhold · 2024-09-17T10:32:28Z

code/core/src/router/utils.ts

+    case '%20':
+      return '+';


Based on this test above, I think we need to decode both + and space here, but I can't figure out if that will be losy: https://github.com/storybookjs/storybook/pull/28315/files#diff-64b8b645dd34caca610919aa0a64fec210cbce8ca52bbb9a6d10b05d32f72ac4R61

Suggested change

case '%20':

return '+';

case '%20': // space

case '%2B': // +

return '+';

Either way, I don't think it matters right now, because when trying out a Date-control on next, a Date arg always turns into the UNIX time number , and never the special value !date(ISO...).

It's probably because of these lines, which have been here since the very beginning: https://github.com/storybookjs/storybook/blob/next/code/lib/blocks/src/controls/Date.tsx/#L93-L96

I'm assuming this is a bug still @ghengeveld?

yeah I suspect if we decode +, we lose the knowledge of if it was a space or a plus. but maybe that loss is ok since we treat both as a space? 🤔

I'm not really sure of the best thing to do here. you will know better than I do, so I'm happy to go with whatever you and the others decide

ndelangen · 2024-09-25T13:52:16Z

code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts

-    const args = parseArgsParam('key:!date(2001-02-03T04:05:06.789+09:00)');
+    const args = parseArgsParam('key:!date(2001-02-03T04:05:06.789%2B09:00)');


Are all these updated tests not indications that this is backwards incompatible, and we should delay merging this until the next major version @JReinhold ?

I don't agree with this updated format to begin with. + is valid in a URL. Moreover, this is fully client-side URL parsing so we don't even have to adhere to the specs necessarily.

feat: migrate from qs

050044f

Moves the `manager-api`, `preview-api` and `ui/amanger` to use `picoquery` instead of `qs` - a much smaller and faster alternative. Note that we still have `qs` in our overall monorepo tree, but that can be tackled in a follow-up PR.

43081j force-pushed the pq branch from fd1e618 to 050044f Compare June 23, 2024 09:40

43081j marked this pull request as ready for review June 23, 2024 10:57

shilman changed the title ~~feat: migrate from qs~~ Core: Migrate from qs to picoquery Jun 23, 2024

shilman added dependencies core ci:normal and removed ci:normal labels Jun 23, 2024

shilman added 2 commits June 23, 2024 21:52

Use destructured imports

2b8156a

qs => picoquery

23f28dc

Merge branch 'next' into pq

a8b3f91

43081j mentioned this pull request Jun 23, 2024

remove qs from storybook es-tooling/ecosystem-cleanup#30

Open

yannbf assigned shilman Jun 24, 2024

feat: use js syntax in query strings

3c04a1e

Uses the `foo.bar[0]` syntax picoquery provides, rather than dot-syntax (`foo.bar.0`).

feat (preview-api): move to picoquery

4474a04

Moves the preview api to use picoquery. Note that tests will not pass yet.

ghengeveld requested a review from ndelangen June 25, 2024 11:50

ghengeveld requested changes Jun 25, 2024

View reviewed changes

43081j added 2 commits June 25, 2024 17:20

Merge branch 'next' into pq

c5f7409

fix: decode some known query string chars

fcce3bb

Decodes certain chars we want to keep in our URIs (e.g. `[`).

Merge branch 'next' into pq

e5cadf3

Merge branch 'next' into pr/43081j/28315

e04c240

43081j and others added 5 commits July 28, 2024 10:57

test: fix parseArgsParam tests

13053d7

Merge branch 'next' into pq

0228c5e

test: add prefix to test queries

5171010

Merge branch 'next' into pr/43081j/28315

c9812f4

dedupe

8f6ff03

ndelangen requested a review from ghengeveld August 2, 2024 20:58

43081j and others added 4 commits August 6, 2024 10:55

Merge branch 'next' into pq

adb5af9

Merge branch 'next' into pr/43081j/28315

a6ebd39

Merge branch 'next' into pq

b51dde1

Merge branch 'next' into pq

8dcfbd0

JReinhold mentioned this pull request Sep 11, 2024

[Tracking] Reduce install footprint 🐾 #29038

Open

Merge branch 'next' into pq

5e9b06e

JReinhold reviewed Sep 17, 2024

View reviewed changes

JReinhold assigned JReinhold and 43081j and unassigned shilman Sep 17, 2024

Merge branch 'next' into pr/43081j/28315

a770d1a

ndelangen reviewed Sep 25, 2024

View reviewed changes

ndelangen added 2 commits September 25, 2024 15:53

rename variable to indicate we're not using qs anymore

a76e060

dedupe

4468a15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Migrate from `qs` to `picoquery` #28315

Core: Migrate from `qs` to `picoquery` #28315

43081j commented Jun 23, 2024 •

edited

Loading

nx-cloud bot commented Jun 23, 2024 •

edited

Loading

shilman commented Jun 23, 2024

43081j commented Jun 23, 2024

shilman commented Jun 23, 2024

43081j commented Jun 23, 2024

43081j commented Jun 24, 2024

ghengeveld left a comment

ghengeveld Jun 25, 2024

ghengeveld Jun 25, 2024

43081j commented Jun 25, 2024 •

edited

Loading

ghengeveld commented Jun 25, 2024

43081j commented Jun 25, 2024

43081j commented Jun 25, 2024

ghengeveld commented Jun 25, 2024 •

edited

Loading

43081j commented Jun 28, 2024

43081j commented Jul 28, 2024 •

edited

Loading

JReinhold Sep 17, 2024

JReinhold Sep 17, 2024

43081j Sep 18, 2024

ndelangen Sep 25, 2024

ghengeveld Sep 25, 2024

		export const queryFromLocation = (location: Partial<Location>) =>
		queryFromString(location.search ? location.search.slice(1) : '');


		export const stringifyQueryParams = (queryParams: Record<string, string>) =>
		qs.stringify(queryParams, { addQueryPrefix: true, encode: false }).replace(/^\?/, '&');

		const args = parseArgsParam('key:!date(2001-02-03T04:05:06.789+09:00)');
		const args = parseArgsParam('key:!date(2001-02-03T04:05:06.789%2B09:00)');

Core: Migrate from qs to picoquery #28315

Are you sure you want to change the base?

Core: Migrate from qs to picoquery #28315

Conversation

43081j commented Jun 23, 2024 • edited Loading

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

nx-cloud bot commented Jun 23, 2024 • edited Loading

☁️ Nx Cloud Report

shilman commented Jun 23, 2024

43081j commented Jun 23, 2024

shilman commented Jun 23, 2024

43081j commented Jun 23, 2024

43081j commented Jun 24, 2024

ghengeveld left a comment

Choose a reason for hiding this comment

ghengeveld Jun 25, 2024

Choose a reason for hiding this comment

ghengeveld Jun 25, 2024

Choose a reason for hiding this comment

43081j commented Jun 25, 2024 • edited Loading

ghengeveld commented Jun 25, 2024

43081j commented Jun 25, 2024

43081j commented Jun 25, 2024

ghengeveld commented Jun 25, 2024 • edited Loading

43081j commented Jun 28, 2024

43081j commented Jul 28, 2024 • edited Loading

JReinhold Sep 17, 2024

Choose a reason for hiding this comment

JReinhold Sep 17, 2024

Choose a reason for hiding this comment

43081j Sep 18, 2024

Choose a reason for hiding this comment

ndelangen Sep 25, 2024

Choose a reason for hiding this comment

ghengeveld Sep 25, 2024

Choose a reason for hiding this comment

Core: Migrate from `qs` to `picoquery` #28315

Core: Migrate from `qs` to `picoquery` #28315

43081j commented Jun 23, 2024 •

edited

Loading

nx-cloud bot commented Jun 23, 2024 •

edited

Loading

43081j commented Jun 25, 2024 •

edited

Loading

ghengeveld commented Jun 25, 2024 •

edited

Loading

43081j commented Jul 28, 2024 •

edited

Loading