Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/silent-countries-jump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@clerk/clerk-js": patch
---

Remove the qs library and use the native URLSearchParams API instead.
3 changes: 1 addition & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions packages/clerk-js/bundlewatch.config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"files": [
{ "path": "./dist/clerk.browser.js", "maxSize": "72kB" },
{ "path": "./dist/clerk.headless.js", "maxSize": "48kB" },
{ "path": "./dist/clerk.browser.js", "maxSize": "62kB" },
{ "path": "./dist/clerk.headless.js", "maxSize": "43kB" },
{ "path": "./dist/ui-common*.js", "maxSize": "85KB" },
{ "path": "./dist/vendors*.js", "maxSize": "70KB" },
{ "path": "./dist/createorganization*.js", "maxSize": "5KB" },
Expand Down
2 changes: 0 additions & 2 deletions packages/clerk-js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
"core-js": "3.26.1",
"dequal": "2.0.3",
"qrcode.react": "3.1.0",
"qs": "6.11.0",
Comment thread
nikosdouvlis marked this conversation as resolved.
"regenerator-runtime": "0.13.11"
},
"devDependencies": {
Expand All @@ -77,7 +76,6 @@
"@clerk/eslint-config-custom": "*",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.10",
"@svgr/webpack": "^6.2.1",
"@types/qs": "^6.9.3",
"@types/react": "*",
"@types/react-dom": "*",
"@types/webpack-dev-server": "^4.7.2",
Expand Down
27 changes: 26 additions & 1 deletion packages/clerk-js/src/core/__tests__/fapiClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,45 @@ describe('buildUrl(options)', () => {
);
});

it('correctly parses search params', () => {
it('parses search params is an object with string values', () => {
expect(fapiClient.buildUrl({ path: '/foo', search: { test: '1' } }).href).toBe(
'https://clerk.example.com/v1/foo?test=1&_clerk_js_version=42.0.0',
);
});

it('parses string search params ', () => {
expect(fapiClient.buildUrl({ path: '/foo', search: 'test=2' }).href).toBe(
'https://clerk.example.com/v1/foo?test=2&_clerk_js_version=42.0.0',
);
});

it('parses search params when value contains invalid url symbols', () => {
expect(fapiClient.buildUrl({ path: '/foo', search: { bar: 'test=2' } }).href).toBe(
'https://clerk.example.com/v1/foo?bar=test%3D2&_clerk_js_version=42.0.0',
);
});

it('parses search params when value is an array', () => {
expect(
fapiClient.buildUrl({
path: '/foo',
search: {
array: ['item1', 'item2'],
},
}).href,
).toBe('https://clerk.example.com/v1/foo?array=item1&array=item2&_clerk_js_version=42.0.0');
});

// The return value isn't as expected.
// The buildUrl function converts an undefined value to the string 'undefined'
// and includes it in the search parameters.
it.skip('parses search params when value is undefined', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we unskip this?

@EmmanouelaPothitou EmmanouelaPothitou Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The buildUrl function converts an undefined value to the string 'undefined' and includes it in the search parameters. This issue existed before removing the qs library, so I didn't address it in this PR.
The qs library used to exclude keys with undefined values from the search parameter string. The stringifyQueryParams function should behave like qs.

I will now create a task in Linear to investigate it further. Therefore, I think we should skip this test for now.

expect(
fapiClient.buildUrl({
path: '/foo',
search: {
array: ['item1', 'item2'],
test: undefined,
},
}).href,
).toBe('https://clerk.example.com/v1/foo?array=item1&array=item2&_clerk_js_version=42.0.0');
Expand Down
17 changes: 9 additions & 8 deletions packages/clerk-js/src/core/fapiClient.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { camelToSnake, isBrowserOnline, runWithExponentialBackOff } from '@clerk/shared';
import type { Clerk, ClerkAPIErrorJSON, ClientJSON } from '@clerk/types';
import qs from 'qs';

import { buildEmailAddress as buildEmailAddressUtil, buildURL as buildUrlUtil } from '../utils';
import { buildEmailAddress as buildEmailAddressUtil, buildURL as buildUrlUtil, stringifyQueryParams } from '../utils';
import { clerkNetworkError } from './errors';

export type HTTPMethod = 'CONNECT' | 'DELETE' | 'GET' | 'HEAD' | 'OPTIONS' | 'PATCH' | 'POST' | 'PUT' | 'TRACE';
Expand Down Expand Up @@ -32,10 +31,6 @@ export type FapiRequestCallback<T> = (
response?: FapiResponse<T>,
) => Promise<unknown | false> | unknown | false;

const camelToSnakeEncoder: qs.IStringifyOptions['encoder'] = (str, defaultEncoder, _, type) => {
return type === 'key' ? camelToSnake(str) : defaultEncoder(str);
};

// TODO: Move to @clerk/types
export interface FapiResponseJSON<T> {
response: T;
Expand Down Expand Up @@ -125,7 +120,7 @@ export function createFapiClient(clerkInstance: Clerk): FapiClient {
return acc;
}, {} as FapiQueryStringParameters & Record<string, string | string[]>);

return qs.stringify(objParams, { addQueryPrefix: true, arrayFormat: 'repeat' });
return stringifyQueryParams(objParams);
}

function buildUrl(requestInit: FapiRequestInit): URL {
Expand Down Expand Up @@ -193,8 +188,14 @@ export function createFapiClient(clerkInstance: Clerk): FapiClient {
// Currently, this is needed only for form-urlencoded, so that the values reach the server in the form
// foo=bar&baz=bar&whatever=1
// @ts-ignore

if (requestInit.headers.get('content-type') === 'application/x-www-form-urlencoded') {
requestInit.body = qs.stringify(body, { encoder: camelToSnakeEncoder, indices: false });
// The native BodyInit type is too wide for our use case,
// so we're casting it to a more specific type here.
// This is covered by the test suite.
requestInit.body = body
? stringifyQueryParams(body as any as Record<string, string>, { keyEncoder: camelToSnake })
: body;
}

const beforeRequestCallbacksResult = await runBeforeRequestCallbacks(requestInit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { RedirectUrls } from '../../utils/redirectUrls';
import { ORGANIZATION_PROFILE_NAVBAR_ROUTE_ID } from '../constants';
import { useEnvironment, useOptions } from '../contexts';
import type { NavbarRoute } from '../elements';
import type { ParsedQs } from '../router';
import type { ParsedQueryString } from '../router';
import { useRouter } from '../router';
import type {
AvailableComponentCtx,
Expand Down Expand Up @@ -44,7 +44,7 @@ const getInitialValuesFromQueryParams = (queryString: string, params: string[])

export type SignUpContextType = SignUpCtx & {
navigateAfterSignUp: () => any;
queryParams: ParsedQs;
queryParams: ParsedQueryString;
signInUrl: string;
signUpUrl: string;
secondFactorUrl: string;
Expand Down Expand Up @@ -115,7 +115,7 @@ export const useSignUpContext = (): SignUpContextType => {

export type SignInContextType = SignInCtx & {
navigateAfterSignIn: () => any;
queryParams: ParsedQs;
queryParams: ParsedQueryString;
signUpUrl: string;
signInUrl: string;
signUpContinueUrl: string;
Expand Down Expand Up @@ -203,7 +203,7 @@ type PagesType = {
};

export type UserProfileContextType = UserProfileCtx & {
queryParams: ParsedQs;
queryParams: ParsedQueryString;
authQueryString: string | null;
pages: PagesType;
};
Expand Down
6 changes: 3 additions & 3 deletions packages/clerk-js/src/ui/router/BaseRouter.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { useClerk } from '@clerk/shared/react';
import type { NavigateOptions } from '@clerk/types';
import qs from 'qs';
import React from 'react';

import { getQueryParams, trimTrailingSlash } from '../../utils';
import { getQueryParams, stringifyQueryParams, trimTrailingSlash } from '../../utils';
import { useWindowEventListener } from '../hooks';
import { newPaths } from './newPaths';
import { match } from './pathToRegexp';
Expand Down Expand Up @@ -113,7 +112,8 @@ export const BaseRouter = ({
toQueryParams[param] = currentQueryParams[param];
}
});
toURL.search = qs.stringify(toQueryParams);

toURL.search = stringifyQueryParams(toQueryParams);
}
const internalNavRes = await internalNavigate(toURL, { metadata: { navigationType: 'internal' } });
setRouteParts({ path: toURL.pathname, queryString: toURL.search });
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/ui/router/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ export * from './VirtualRouter';
export * from './Route';
export * from './Switch';

export type { ParsedQs } from 'qs';
export type ParsedQueryString = Record<string, string>;
69 changes: 64 additions & 5 deletions packages/clerk-js/src/utils/__tests__/querystring.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,75 @@
import { camelToSnake } from '@clerk/shared';

import { getQueryParams, stringifyQueryParams } from '../querystring';

describe('getQueryParams(string)', () => {
it('parses a querystring', () => {
expect(getQueryParams('')).toEqual({});
expect(getQueryParams('foo=42&bar=43')).toEqual({ foo: '42', bar: '43' });
expect(getQueryParams('?foo=42&bar=43')).toEqual({ foo: '42', bar: '43' });
it('parses an emtpy querystring', () => {
const res = getQueryParams('');
expect(res).toEqual({});
});

it('parses a querystring into a URLSearchParams instance', () => {
const res = getQueryParams('foo=42&bar=43');
expect(res).toEqual({ foo: '42', bar: '43' });
});

it('parses a querystring into a URLSearchParams instance even when prefixed with ?', () => {
const res = getQueryParams('?foo=42&bar=43');
expect(res).toEqual({ foo: '42', bar: '43' });
});

it('parses multiple occurances of the same key as an array', () => {
const res = getQueryParams('?foo=42&foo=43&bar=1');
expect(res).toEqual({ foo: ['42', '43'], bar: '1' });
});
});

describe('stringifyQueryParams(object)', () => {
it('handles null values', () => {
expect(stringifyQueryParams(null)).toBe('');
});

it('handles undefined values', () => {
expect(stringifyQueryParams(undefined)).toBe('');
});

it('handles string values', () => {
expect(stringifyQueryParams('hello')).toBe('');
});

it('handles empty string values', () => {
expect(stringifyQueryParams('')).toBe('');
});

it('converts an object to querystring', () => {
expect(stringifyQueryParams({})).toEqual('');
expect(stringifyQueryParams({ foo: '42', bar: '43' })).toBe('foo=42&bar=43');
});

it('converts an object to querystring when value is an array', () => {
expect(stringifyQueryParams({ foo: ['42', '22'], bar: '43' })).toBe('foo=42&foo=22&bar=43');
});

it('converts an object to querystring when value is undefined', () => {
expect(stringifyQueryParams({ foo: ['42', '22'], bar: undefined })).toBe('foo=42&foo=22');
});

it('converts an object to querystring when value is null', () => {
expect(stringifyQueryParams({ foo: null })).toBe('foo=');
});

it('converts an object to querystring when value is an object', () => {
expect(stringifyQueryParams({ unsafe_metadata: { bar: '1' } })).toBe('unsafe_metadata=%7B%22bar%22%3A%221%22%7D');
});

it('converts an object to querystring when value contains invalid url symbols', () => {
expect(stringifyQueryParams({ test: 'ena=duo' })).toBe('test=ena%3Dduo');
});

it('converts an object to querystring when key is camelCase', () => {
expect(stringifyQueryParams({ barFoo: '1' }, { keyEncoder: camelToSnake })).toBe('bar_foo=1');
expect(stringifyQueryParams({ unsafeMetadata: { bar: '1' } }, { keyEncoder: camelToSnake })).toBe(
'unsafe_metadata=%7B%22bar%22%3A%221%22%7D',
);
});
});
//test=ena%3Dduo
59 changes: 52 additions & 7 deletions packages/clerk-js/src/utils/querystring.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,56 @@
import qs from 'qs';

export const getQueryParams = (queryString: string) => {
return qs.parse(queryString || '', {
ignoreQueryPrefix: true,
}) as Record<string, string>;
const queryParamsObject: { [key: string]: string | string[] } = {};
const queryParams = new URLSearchParams(queryString);
queryParams.forEach((value, key) => {
if (key in queryParamsObject) {
// If the key already exists, we need to handle it as an array
const existingValue = queryParamsObject[key];
if (Array.isArray(existingValue)) {
existingValue.push(value);
} else {
queryParamsObject[key] = [existingValue, value];
}
} else {
queryParamsObject[key] = value;
}
});
return queryParamsObject as Record<string, string>;
Comment thread
EmmanouelaPothitou marked this conversation as resolved.
};

type StringifyQueryParamsOptions = {
keyEncoder?: (key: string) => string;
};

export const stringifyQueryParams = (params: Record<string, unknown> | Array<unknown>) => {
return qs.stringify(params || {});
export const stringifyQueryParams = (
params:
| Record<string, string | undefined | null | object | Array<string | undefined | null>>
| null
| undefined
| string,
opts: StringifyQueryParamsOptions = {},
) => {
if (params === null || params === undefined) {
return '';
}
if (!params || typeof params !== 'object') {
return '';
}

const queryParams = new URLSearchParams();

Object.keys(params).forEach(key => {
const encodedKey = opts.keyEncoder ? opts.keyEncoder(key) : key;
const value = params[key];
if (Array.isArray(value)) {
value.forEach(v => v !== undefined && queryParams.append(encodedKey, v || ''));
} else if (value === undefined) {
return;
} else if (typeof value === 'object' && value !== null) {
queryParams.append(encodedKey, JSON.stringify(value));
} else {
queryParams.append(encodedKey, value || '');
}
});

return queryParams.toString();
};