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.

Sign up for GitHub

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

refactor!: remove ufo dependency #440

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

refactor!: remove ufo dependency #440

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc714ee
feat!: replace `ufo` with native `URL` utils
johannschopplich Sep 5, 2024
997afbb
fix: reduce `URL` usage and fix tests
johannschopplich Sep 5, 2024
ad22663
refactor: share `http://localhost` string
johannschopplich Sep 5, 2024
d7075be
perf: drop `URL` usage in `withQuery`
johannschopplich Sep 5, 2024
8478387
perf: return early if no query params in input URL
johannschopplich Sep 5, 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
Prev Previous commit
Next Next commit
perf: drop URL usage in withQuery
  • Loading branch information
@johannschopplich
johannschopplich committed Sep 5, 2024
commit d7075bebf2034c9f60ead847b70b4f7254605c30
30 changes: 9 additions & 21 deletions src/path.ts
View file
Original file line number Diff line number Diff line change
@@ -9,26 +9,24 @@
| undefined;
export type QueryObject = Record<string, QueryValue | QueryValue[]>;

const DEFAULT_BASE_URL = "http://localhost";

/**
* Removes the leading slash from the given path if it has one.
*/
export function withoutLeadingSlash(path?: string): string {
if (!path || path === "/") {
return "/";
}

Check warning on line 18 in src/path.ts

Codecov / codecov/patch

src/path.ts#L16-L18 

Added lines #L16 - L18 were not covered by tests

return path[0] === "/" ? path.slice(1) : path;
}

Check warning on line 21 in src/path.ts

Codecov / codecov/patch

src/path.ts#L20-L21 

Added lines #L20 - L21 were not covered by tests

/**
* Removes the trailing slash from the given path if it has one.
*/
export function withoutTrailingSlash(path?: string): string {
if (!path || path === "/") {
return "/";
}

Check warning on line 29 in src/path.ts

Codecov / codecov/patch

src/path.ts#L28-L29 

Added lines #L28 - L29 were not covered by tests

return path[path.length - 1] === "/" ? path.slice(0, -1) : path;
}
@@ -38,8 +36,8 @@
*/
export function joinURL(base?: string, path?: string): string {
if (!base || base === "/") {
return path || "/";
}

Check warning on line 40 in src/path.ts

Codecov / codecov/patch

src/path.ts#L39-L40 

Added lines #L39 - L40 were not covered by tests

if (!path || path === "/") {
return base || "/";
@@ -48,11 +46,11 @@
const baseHasTrailing = base[base.length - 1] === "/";
const pathHasLeading = path[0] === "/";
if (baseHasTrailing && pathHasLeading) {
return base + path.slice(1);

Check warning on line 49 in src/path.ts

Codecov / codecov/patch

src/path.ts#L49 

Added line #L49 was not covered by tests
}

if (!baseHasTrailing && !pathHasLeading) {
return `${base}/${path}`;

Check warning on line 53 in src/path.ts

Codecov / codecov/patch

src/path.ts#L53 

Added line #L53 was not covered by tests
}

return base + path;
@@ -63,13 +61,13 @@
*/
export function withBase(input = "", base = ""): string {
if (!base || base === "/") {
return input;
}

Check warning on line 65 in src/path.ts

Codecov / codecov/patch

src/path.ts#L64-L65 

Added lines #L64 - L65 were not covered by tests

const _base = withoutTrailingSlash(base);
if (input.startsWith(_base)) {
return input;
}

Check warning on line 70 in src/path.ts

Codecov / codecov/patch

src/path.ts#L69-L70 

Added lines #L69 - L70 were not covered by tests

return joinURL(_base, input);
}
@@ -78,44 +76,34 @@
* Returns the URL with the given query parameters. If a query parameter is undefined, it is omitted.
*/
export function withQuery(input: string, query: QueryObject): string {
let url: URL | undefined;
let searchParams: URLSearchParams;

if (input.includes("?")) {
url = new URL(input, DEFAULT_BASE_URL);
searchParams = new URLSearchParams(url.search);
} else {
searchParams = new URLSearchParams();
if (!query || Object.keys(query).length === 0) {
return input;
}

Check warning on line 81 in src/path.ts

Codecov / codecov/patch

src/path.ts#L80-L81 

Added lines #L80 - L81 were not covered by tests

const searchIndex = input.indexOf("?");
const base = searchIndex === -1 ? input : input.slice(0, searchIndex);
const searchParams = new URLSearchParams(
searchIndex === -1 ? "" : input.slice(searchIndex + 1)
Copy link
Member

Choose a reason for hiding this comment

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

fast-path: when there is no searchIndex we can avoid creating new URLSearchParams (sorry for being strict this is kind of thing we forget later and it can be potentially bases to ufo/lite trying to make sure optimizing it as much as we can)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't get it completely: Even if no search is in the input path, we still need to construct the encoded query string for the new output path, am I right?
How should we build the search params if not using URLSearchParams? Port the stringifyQuery function from ufo?

P.S.: Of course, no need to justify optimization and future-proofness. 🙋‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Sorry my wording was confusing. I meant that when there is no quesry in input string, we can have a fast path if block. So we don't need to iterate to for example remove undefined values, etc (we already know query state is empty) so it can be something like return new URLSearchParams(query).toString (I'm not sure if query object needs normalization but it could be simple map)

Copy link
Contributor Author

@johannschopplich johannschopplich Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, I get it now! Thanks for the clarification. Now:

  1. If the query is empty, we return early.
  2. If the input URL has no query, we only normalize the items of the input query object.

Is that what you intended?

We have to normalize the values for URLSearchParams, otherwise it returns string like key=undefined. I have again tested with the (slighty changed) ufo tests. I will create a separate PR there to track the implementation of these lighter functions.

);

for (const [key, value] of Object.entries(query)) {
if (value === undefined) {
searchParams.delete(key);

Check warning on line 91 in src/path.ts

Codecov / codecov/patch

src/path.ts#L91 

Added line #L91 was not covered by tests
} else if (typeof value === "number" || typeof value === "boolean") {
searchParams.set(key, String(value));
} else if (!value) {
searchParams.set(key, "");
} else if (Array.isArray(value)) {
for (const item of value) {
searchParams.append(key, String(item));
}
} else if (typeof value === "object") {
searchParams.set(key, JSON.stringify(value));
} else {
searchParams.set(key, String(value));
}

Check warning on line 104 in src/path.ts

Codecov / codecov/patch

src/path.ts#L95-L104 

Added lines #L95 - L104 were not covered by tests
}

const queryString = searchParams.toString();

if (url) {
url.search = queryString;
let urlWithQuery = url.toString();
if (urlWithQuery.startsWith(DEFAULT_BASE_URL)) {
urlWithQuery = urlWithQuery.slice(16);
}
return urlWithQuery;
}

return queryString ? `${input}?${queryString}` : input;
return queryString ? `${base}?${queryString}` : base;
}