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

Core: Migrate from qs to picoquery #28315

Merged
merged 33 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
050044f
feat: migrate from qs
43081j Jun 23, 2024
2b8156a
Use destructured imports
shilman Jun 23, 2024
23f28dc
qs => picoquery
shilman Jun 23, 2024
a8b3f91
Merge branch 'next' into pq
shilman Jun 23, 2024
3c04a1e
feat: use js syntax in query strings
43081j Jun 24, 2024
4474a04
feat (preview-api): move to picoquery
43081j Jun 24, 2024
c5f7409
Merge branch 'next' into pq
43081j Jun 25, 2024
fcce3bb
fix: decode some known query string chars
43081j Jun 25, 2024
e5cadf3
Merge branch 'next' into pq
ndelangen Jun 28, 2024
e04c240
Merge branch 'next' into pr/43081j/28315
ndelangen Jul 12, 2024
13053d7
test: fix parseArgsParam tests
43081j Jul 28, 2024
0228c5e
Merge branch 'next' into pq
43081j Jul 28, 2024
5171010
test: add prefix to test queries
43081j Jul 28, 2024
c9812f4
Merge branch 'next' into pr/43081j/28315
ndelangen Aug 2, 2024
8f6ff03
dedupe
ndelangen Aug 2, 2024
adb5af9
Merge branch 'next' into pq
43081j Aug 6, 2024
a6ebd39
Merge branch 'next' into pr/43081j/28315
ndelangen Aug 14, 2024
b51dde1
Merge branch 'next' into pq
43081j Aug 18, 2024
8dcfbd0
Merge branch 'next' into pq
43081j Sep 7, 2024
5e9b06e
Merge branch 'next' into pq
JReinhold Sep 17, 2024
a770d1a
Merge branch 'next' into pr/43081j/28315
ndelangen Sep 25, 2024
a76e060
rename variable to indicate we're not using `qs` anymore
ndelangen Sep 25, 2024
4468a15
dedupe
ndelangen Sep 25, 2024
f8c9172
Discard changes to code/core/src/preview-api/modules/preview-web/pars…
JReinhold Sep 26, 2024
b45ea68
fix parseArgsParams and tests
JReinhold Sep 26, 2024
099be6b
format
JReinhold Sep 26, 2024
87bc10d
Merge branch 'next' into pr/43081j/28315
ndelangen Sep 26, 2024
c08542b
fix
ndelangen Sep 26, 2024
df1aad2
Fixes by not re-using query-options meant for args&globals use only
ndelangen Sep 30, 2024
6066c64
make our color examples lowercase always & fix a bug where we didn't …
ndelangen Sep 30, 2024
1f43562
fix
ndelangen Sep 30, 2024
be07741
Merge branch 'next' into pr/43081j/28315
ndelangen Sep 30, 2024
26659e4
Merge branch 'next' into pq
ndelangen Sep 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion code/lib/manager-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
},
"devDependencies": {
"@types/lodash": "^4.14.167",
"@types/qs": "^6",
"@types/semver": "^7.3.4",
"flush-promises": "^1.0.2",
"react": "^18.2.0",
Expand Down
3 changes: 1 addition & 2 deletions code/lib/preview-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,10 @@
"@storybook/csf": "^0.1.8",
"@storybook/global": "^5.0.0",
"@storybook/types": "workspace:*",
"@types/qs": "^6.9.5",
"dequal": "^2.0.2",
"lodash": "^4.17.21",
"memoizerific": "^1.11.3",
"qs": "^6.10.0",
"picoquery": "^1.3.0",
"tiny-invariant": "^1.3.1",
"ts-dedent": "^2.0.0",
"util-deprecate": "^1.0.2"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ export interface SelectionStore {

setSelection(selection: Selection): void;

setQueryParams(queryParams: qs.ParsedQs): void;
setQueryParams(queryParams: Record<PropertyKey, unknown>): void;
}
4 changes: 2 additions & 2 deletions code/lib/preview-api/src/modules/preview-web/UrlStore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('UrlStore', () => {
);
});
it('should replace legacy parameters but preserve others', () => {
document.location.search = 'foo=bar&selectedStory=selStory&selectedKind=selKind';
document.location.search = '?foo=bar&selectedStory=selStory&selectedKind=selKind';
setPath({ storyId: 'story--id', viewMode: 'story' });
expect(history.replaceState).toHaveBeenCalledWith(
{},
Expand All @@ -48,7 +48,7 @@ describe('UrlStore', () => {
);
});
it('should ignore + keep hashes', () => {
document.location.search = 'foo=bar&selectedStory=selStory&selectedKind=selKind';
document.location.search = '?foo=bar&selectedStory=selStory&selectedKind=selKind';
document.location.hash = '#foobar';
setPath({ storyId: 'story--id', viewMode: 'story' });
expect(history.replaceState).toHaveBeenCalledWith(
Expand Down
36 changes: 20 additions & 16 deletions code/lib/preview-api/src/modules/preview-web/UrlStore.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { global } from '@storybook/global';
import qs from 'qs';
import { parse, stringify } from 'picoquery';
import type { ViewMode } from '@storybook/types';

import { parseArgsParam } from './parseArgsParam';
Expand All @@ -20,19 +20,20 @@ const getQueryString = ({
extraParams,
}: {
selection?: Selection;
extraParams?: qs.ParsedQs;
extraParams?: Record<PropertyKey, unknown>;
}) => {
const search = typeof document !== 'undefined' ? document.location.search : '';
const { path, selectedKind, selectedStory, ...rest } = qs.parse(search, {
ignoreQueryPrefix: true,
});
return qs.stringify(
{
const search =
typeof document !== 'undefined' && document.location.search
? document.location.search.slice(1)
: '';
43081j marked this conversation as resolved.
Show resolved Hide resolved
const { path, selectedKind, selectedStory, ...rest } = parse(search);
return (
'?' +
stringify({
43081j marked this conversation as resolved.
Show resolved Hide resolved
...rest,
...extraParams,
...(selection && { id: selection.storyId, viewMode: selection.viewMode }),
},
{ encode: false, addQueryPrefix: true }
})
);
};

Expand All @@ -45,10 +46,10 @@ export const setPath = (selection?: Selection) => {
};

type ValueOf<T> = T[keyof T];
const isObject = (val: Record<string, any>) =>
const isObject = (val: Record<string, any>): val is object =>
val != null && typeof val === 'object' && Array.isArray(val) === false;

const getFirstString = (v: ValueOf<qs.ParsedQs>): string | void => {
const getFirstString = (v: ValueOf<Record<PropertyKey, unknown>>): string | void => {
if (v === undefined) {
return undefined;
}
Expand All @@ -58,15 +59,18 @@ const getFirstString = (v: ValueOf<qs.ParsedQs>): string | void => {
if (Array.isArray(v)) {
return getFirstString(v[0]);
}
if (isObject(v)) {
return getFirstString(Object.values(v).filter(Boolean) as string[]);
if (isObject(v as Record<PropertyKey, unknown>)) {
return getFirstString(
Object.values(v as Record<PropertyKey, unknown>).filter(Boolean) as string[]
);
}
return undefined;
};

export const getSelectionSpecifierFromPath: () => SelectionSpecifier | null = () => {
if (typeof document !== 'undefined') {
const query = qs.parse(document.location.search, { ignoreQueryPrefix: true });
const queryStr = document.location.search ? document.location.search.slice(1) : '';
const query = parse(queryStr);
43081j marked this conversation as resolved.
Show resolved Hide resolved
const args = typeof query.args === 'string' ? parseArgsParam(query.args) : undefined;
const globals = typeof query.globals === 'string' ? parseArgsParam(query.globals) : undefined;

Expand Down Expand Up @@ -100,7 +104,7 @@ export class UrlStore implements SelectionStore {
setPath(this.selection);
}

setQueryParams(queryParams: qs.ParsedQs) {
setQueryParams(queryParams: Record<PropertyKey, unknown>) {
const query = getQueryString({ extraParams: queryParams });
const { hash = '' } = document.location;
history.replaceState({}, '', `${document.location.pathname}${query}${hash}`);
Expand Down
6 changes: 2 additions & 4 deletions code/lib/preview-api/src/modules/preview-web/WebView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { global } from '@storybook/global';
import { logger } from '@storybook/client-logger';
import AnsiToHtml from 'ansi-to-html';
import { dedent } from 'ts-dedent';
import qs from 'qs';
import { parse } from 'picoquery';

import type { PreparedStory } from '@storybook/types';
import type { View } from './View';
Expand Down Expand Up @@ -48,9 +48,7 @@ export class WebView implements View<HTMLElement> {
// Special code for testing situations
if (typeof document !== 'undefined') {
// eslint-disable-next-line @typescript-eslint/naming-convention
const { __SPECIAL_TEST_PARAMETER__ } = qs.parse(document.location.search, {
ignoreQueryPrefix: true,
});
const { __SPECIAL_TEST_PARAMETER__ } = parse(document.location.search);
switch (__SPECIAL_TEST_PARAMETER__) {
case 'preparing-story': {
this.showPreparingStory();
Expand Down
25 changes: 11 additions & 14 deletions code/lib/preview-api/src/modules/preview-web/parseArgsParam.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import qs from 'qs';
import { parse, type Options } from 'picoquery';
import { dedent } from 'ts-dedent';
import type { Args } from '@storybook/types';
import { once } from '@storybook/client-logger';
Expand Down Expand Up @@ -30,17 +30,14 @@ const validateArgs = (key = '', value: unknown): boolean => {
return false;
};

const QS_OPTIONS = {
const QS_OPTIONS: Partial<Options> = {
delimiter: ';', // we're parsing a single query param
allowDots: true, // objects are encoded using dot notation
allowSparse: true, // arrays will be merged on top of their initial value
decoder(
str: string,
defaultDecoder: (str: string, decoder?: any, charset?: string) => string,
charset: string,
type: 'key' | 'value'
) {
if (type === 'value' && str.startsWith('!')) {
nesting: true,
arrayRepeat: true,
arrayRepeatSyntax: 'bracket',
nestingSyntax: 'js', // objects are encoded using dot notation
valueDeserializer(str: string) {
if (str.startsWith('!')) {
if (str === '!undefined') return undefined;
if (str === '!null') return null;
if (str === '!true') return true;
Expand All @@ -59,13 +56,13 @@ const QS_OPTIONS = {
: `${color[1]}(${color[2]}, ${color[3]}%, ${color[4]}%)`;
}
}
if (type === 'value' && NUMBER_REGEXP.test(str)) return Number(str);
return defaultDecoder(str, defaultDecoder, charset);
if (NUMBER_REGEXP.test(str)) return Number(str);
return str;
},
};
export const parseArgsParam = (argsString: string): Args => {
const parts = argsString.split(';').map((part) => part.replace('=', '~').replace(':', '='));
return Object.entries(qs.parse(parts.join(';'), QS_OPTIONS)).reduce((acc, [key, value]) => {
return Object.entries(parse(parts.join(';'), QS_OPTIONS)).reduce((acc, [key, value]) => {
if (validateArgs(key, value)) return Object.assign(acc, { [key]: value });
once.warn(dedent`
Omitted potentially unsafe URL args.
Expand Down
2 changes: 1 addition & 1 deletion code/lib/router/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"dependencies": {
"@storybook/client-logger": "workspace:*",
"memoizerific": "^1.11.3",
"qs": "^6.10.0"
"picoquery": "^1.3.0"
},
"devDependencies": {
"@storybook/global": "^5.0.0",
Expand Down
26 changes: 13 additions & 13 deletions code/lib/router/src/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,12 @@ describe('buildArgsParam', () => {

it('builds arrays', () => {
const param = buildArgsParam({}, { arr: ['1', '2', '3'] });
expect(param).toEqual('arr[0]:1;arr[1]:2;arr[2]:3');
expect(param).toEqual('arr%5B0%5D:1;arr%5B1%5D:2;arr%5B2%5D:3');
43081j marked this conversation as resolved.
Show resolved Hide resolved
});

it('builds sparse arrays', () => {
const param = buildArgsParam({}, { arr: ['1', , '3'] });
expect(param).toEqual('arr[0]:1;arr[2]:3');
expect(param).toEqual('arr%5B0%5D:1;arr%5B2%5D:3');
});

it('builds simple objects', () => {
Expand All @@ -157,22 +157,22 @@ describe('buildArgsParam', () => {

it('builds arrays in objects', () => {
const param = buildArgsParam({}, { obj: { foo: ['1', , '3'] } });
expect(param).toEqual('obj.foo[0]:1;obj.foo[2]:3');
expect(param).toEqual('obj.foo%5B0%5D:1;obj.foo%5B2%5D:3');
});

it('builds single object in array', () => {
const param = buildArgsParam({}, { arr: [{ one: '1', two: '2' }] });
expect(param).toEqual('arr[0].one:1;arr[0].two:2');
expect(param).toEqual('arr%5B0%5D.one:1;arr%5B0%5D.two:2');
});

it('builds multiple objects in array', () => {
const param = buildArgsParam({}, { arr: [{ one: '1' }, { two: '2' }] });
expect(param).toEqual('arr[0].one:1;arr[1].two:2');
expect(param).toEqual('arr%5B0%5D.one:1;arr%5B1%5D.two:2');
});

it('builds nested object in array', () => {
const param = buildArgsParam({}, { arr: [{ foo: { bar: 'val' } }] });
expect(param).toEqual('arr[0].foo.bar:val');
expect(param).toEqual('arr%5B0%5D.foo.bar:val');
});

it('encodes space as +', () => {
Expand All @@ -187,7 +187,7 @@ describe('buildArgsParam', () => {

it('encodes nested null values as !null', () => {
const param = buildArgsParam({}, { foo: { bar: [{ key: null }], baz: null } });
expect(param).toEqual('foo.bar[0].key:!null;foo.baz:!null');
expect(param).toEqual('foo.bar%5B0%5D.key:!null;foo.baz:!null');
});

it('encodes hex color values as !hex(value)', () => {
Expand All @@ -197,17 +197,17 @@ describe('buildArgsParam', () => {

it('encodes rgba color values by prefixing and compacting', () => {
const param = buildArgsParam({}, { rgb: 'rgb(255, 71, 133)', rgba: 'rgba(255, 71, 133, 0.5)' });
expect(param).toEqual('rgb:!rgb(255,71,133);rgba:!rgba(255,71,133,0.5)');
expect(param).toEqual('rgb:!rgb(255%2C71%2C133);rgba:!rgba(255%2C71%2C133%2C0.5)');
});

it('encodes hsla color values by prefixing and compacting', () => {
const param = buildArgsParam({}, { hsl: 'hsl(45, 99%, 70%)', hsla: 'hsla(45, 99%, 70%, 0.5)' });
expect(param).toEqual('hsl:!hsl(45,99,70);hsla:!hsla(45,99,70,0.5)');
expect(param).toEqual('hsl:!hsl(45%2C99%2C70);hsla:!hsla(45%2C99%2C70%2C0.5)');
});

it('encodes Date objects as !date(ISO string)', () => {
const param = buildArgsParam({}, { key: new Date('2001-02-03T04:05:06.789Z') });
expect(param).toEqual('key:!date(2001-02-03T04:05:06.789Z)');
expect(param).toEqual('key:!date(2001-02-03T04%3A05%3A06.789Z)');
});

describe('with initial state', () => {
Expand All @@ -223,7 +223,7 @@ describe('buildArgsParam', () => {

it('sets !undefined for removed array values', () => {
const param = buildArgsParam({ arr: [1] }, { arr: [] });
expect(param).toEqual('arr[0]:!undefined');
expect(param).toEqual('arr%5B0%5D:!undefined');
});

it('sets !undefined for removed object properties', () => {
Expand All @@ -233,15 +233,15 @@ describe('buildArgsParam', () => {

it('omits unchanged array values (yielding sparse arrays)', () => {
const param = buildArgsParam({ arr: [1, 2, 3] }, { arr: [1, 3, 4] });
expect(param).toEqual('arr[1]:3;arr[2]:4');
expect(param).toEqual('arr%5B1%5D:3;arr%5B2%5D:4');
});

it('omits nested unchanged object properties and array values', () => {
const param = buildArgsParam(
{ obj: { nested: [{ one: 1 }, { two: 2 }] } },
{ obj: { nested: [{ one: 1 }, { two: 2, three: 3 }] } }
);
expect(param).toEqual('obj.nested[1].three:3');
expect(param).toEqual('obj.nested%5B1%5D.three:3');
});
});
});
35 changes: 21 additions & 14 deletions code/lib/router/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { once } from '@storybook/client-logger';
import { dequal as deepEqual } from 'dequal';
import isPlainObject from 'lodash/isPlainObject.js';
import memoize from 'memoizerific';
import type { IStringifyOptions } from 'qs';
import qs from 'qs';
import type { Options as QueryOptions } from 'picoquery';
import { stringify, parse } from 'picoquery';
import { dedent } from 'ts-dedent';

export interface StoryData {
Expand Down Expand Up @@ -92,6 +92,9 @@ const validateArgs = (key = '', value: unknown): boolean => {
return false;
};

// Note this isn't a picoquery serializer because pq will turn any object
// into a nested key internally. So we need to deal witth things like `Date`
// up front.
const encodeSpecialValues = (value: unknown): any => {
if (value === undefined) return '!undefined';
if (value === null) return '!null';
Expand All @@ -105,6 +108,10 @@ const encodeSpecialValues = (value: unknown): any => {
return `!${value}`;
}

if (value instanceof Date) {
return `!date(${value.toISOString()})`;
}

if (Array.isArray(value)) return value.map(encodeSpecialValues);
if (isPlainObject(value)) {
return Object.entries(value as Record<string, any>).reduce(
Expand All @@ -115,12 +122,10 @@ const encodeSpecialValues = (value: unknown): any => {
return value;
};

const QS_OPTIONS: IStringifyOptions = {
encode: false, // we handle URL encoding ourselves
const QS_OPTIONS: Partial<QueryOptions> = {
delimiter: ';', // we don't actually create multiple query params
allowDots: true, // encode objects using dot notation: obj.key=val
format: 'RFC1738', // encode spaces using the + sign
serializeDate: (date: Date) => `!date(${date.toISOString()})`,
nesting: true,
nestingSyntax: 'js', // encode objects using dot notation: obj.key=val
};
export const buildArgsParam = (initialArgs: Args | undefined, args: Args): string => {
const update = deepDiff(initialArgs, args);
Expand All @@ -136,9 +141,8 @@ export const buildArgsParam = (initialArgs: Args | undefined, args: Args): strin
return acc;
}, {} as Args);

return qs
.stringify(encodeSpecialValues(object), QS_OPTIONS)
.replace(/ /g, '+')
return stringify(encodeSpecialValues(object), QS_OPTIONS)
.replace(/%20/g, '+')
.split(';')
.map((part: string) => part.replace('=', ':'))
.join(';');
Expand All @@ -149,11 +153,14 @@ interface Query {
}

export const queryFromString = memoize(1000)(
(s?: string): Query => (s !== undefined ? qs.parse(s, { ignoreQueryPrefix: true }) : {})
(s?: string): Query => (s !== undefined ? parse(s) : {})
);
export const queryFromLocation = (location: Partial<Location>) => queryFromString(location.search);
export const stringifyQuery = (query: Query) =>
qs.stringify(query, { addQueryPrefix: true, encode: false });
export const queryFromLocation = (location: Partial<Location>) =>
queryFromString(location.search ? location.search.slice(1) : '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

export const stringifyQuery = (query: Query) => {
const queryStr = stringify(query);
return queryStr ? '?' + queryStr : '';
};

type Match = { path: string };

Expand Down
2 changes: 1 addition & 1 deletion code/ui/manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@
"lodash": "^4.17.21",
"markdown-to-jsx": "^7.4.5",
"memoizerific": "^1.11.3",
"picoquery": "^1.3.0",
"polished": "^4.2.2",
"qs": "^6.10.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-draggable": "^4.4.5",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import qs from 'qs';
import { stringify } from 'picoquery';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading
Loading