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

refactor!: remove ufo dependency #440

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

Conversation

johannschopplich
Copy link
Contributor

@johannschopplich johannschopplich commented Sep 5, 2024

Note

This PR might be better off over at ufo. Feel free to close this PR if it should be handled otherwise. I just wanted to share the idea and get some feedback. 🙂

Migrating ufo to use native URL is already in discussion, which would be beneficial to reduce the bundle size in the whole UnJS ecosystem, I suppose.

In @pi0 work on h3 v2, ufo is no longer used and path utilities have been internalized to the repo. This might be also an approach for ofetch, since it only uses the following exports:

  • withBase
  • withQuery

Taking inspiration from the refactors in h3 v2, both path utilities have been inlined to remove the dependency on ofetch. At the time of writing, one test still fails.

@pi0 pi0 marked this pull request as draft September 5, 2024 11:52
src/path.ts Outdated Show resolved Hide resolved
@pi0 pi0 changed the title refactor!: replace ufo with native URL utils refactor!: remove ufo dependency Sep 5, 2024
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 53.48837% with 40 lines in your changes missing coverage. Please review.

Project coverage is 67.37%. Comparing base (27996d3) to head (8478387).
Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
src/path.ts 52.94% 40 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #440       +/-   ##
===========================================
+ Coverage   56.86%   67.37%   +10.50%     
===========================================
  Files          16       18        +2     
  Lines         728      610      -118     
  Branches      113      162       +49     
===========================================
- Hits          414      411        -3     
+ Misses        303      188      -115     
  Partials       11       11               

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/path.ts Outdated Show resolved Hide resolved
src/path.ts Show resolved Hide resolved
src/path.ts Outdated
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants