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

simplified approach to ensuring Url type is returned from urlAssembly #308

Merged
merged 8 commits into from
Nov 16, 2024

Conversation

stackoverfloweth
Copy link
Contributor

@stackoverfloweth stackoverfloweth commented Nov 10, 2024

In an attempt to simplify common use cases of route.matches, we discovered some functions that were returning string when they should be returning Url.

In a previous approach, we put it on the developer to make sure the routes they built satisfy the Url type when assembled. However, we couldn't really test routes since routes might have dynamic params. Plus developers probably aren't super familiar with what it means for a route to be "assembled".

This PR takes a much simpler approach by just assembling the url, and then if the assembled url is NOT a valid Url type, it assumes the route must be relative and adds the forward slash.

This is no different from the query, how we automatically add the ?, or hash how we always normalize the value to starting with #. "Paths" (when assembled) are expected the start with /, so if the developer omits it, we simply add it for them.

as a side-quest this PR removes complexity from url parts (path, host, query, hash).

  • these objects are simple containers now, no toString and no hasValue based on potentially stale values
  • logic for cleaning and joining url parts is moved to "withPath", "withHost", etc.

Copy link

netlify bot commented Nov 10, 2024

Deploy Preview for kitbag-router ready!

Name Link
🔨 Latest commit a6d199f
🔍 Latest deploy log https://app.netlify.com/sites/kitbag-router/deploys/6736acdeea07100008144a2d
😎 Deploy Preview https://deploy-preview-308--kitbag-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Overall this looks good and I like the direction.

src/services/urlAssembly.ts Outdated Show resolved Hide resolved
src/services/withPath.ts Outdated Show resolved Hide resolved
src/services/withPath.ts Outdated Show resolved Hide resolved
src/services/withHost.ts Outdated Show resolved Hide resolved
…d maybeRelativeUrl service to also export a function that joins the parts to create a Url.
src/types/url.ts Show resolved Hide resolved
src/services/withQuery.ts Outdated Show resolved Hide resolved
src/services/withQuery.ts Outdated Show resolved Hide resolved
src/services/withPath.spec.ts Outdated Show resolved Hide resolved
src/services/withHost.ts Outdated Show resolved Hide resolved
src/services/withHash.ts Outdated Show resolved Hide resolved
@stackoverfloweth stackoverfloweth merged commit 9489179 into main Nov 16, 2024
6 checks passed
@stackoverfloweth stackoverfloweth deleted the resolve-returns-url-type-simplified branch November 16, 2024 16:17
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