-
-
Couldn't load subscription status.
- Fork 2.1k
feat: add optional params for hash and searchParams to resolve()
#14756
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
base: main
Are you sure you want to change the base?
feat: add optional params for hash and searchParams to resolve()
#14756
Conversation
🦋 Changeset detectedLatest commit: 50af08d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/kit/src/types/ambient.d.ts
Outdated
| */ | ||
| export type ResolveURLParams = { | ||
| hash?: string; | ||
| searchParams?: Record<string, string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Came to this PR from suffering from the huge eslint breakage due to this not being possible. 👍
I'd like to see this be at least searchParams?: Record<string, string> | URLSearchParams;.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense. Happy to implement that too, I'm just a bit lost as to why the tests are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkarikos I implemented your suggestion!
751337e to
da2e1b1
Compare
Adds optional arguments for URL params `hash` and `searchParams` to `resolve()`. Appends these to the returned URL after resolving the path. Resolves sveltejs#14750 and helps address sveltejs/eslint-plugin-svelte#1353 feat: make ResolveParams accept `searchParams: Record<string, string> | URLSearchParams | undefined` fix: better arg parsing
da2e1b1 to
6d27fae
Compare
|
Any pointers as to why When I try to see the details, I just get met with a |
Please go easy on me as this is the first PR I'm making to this repository, I am more than open to any suggested changes. I've been trying to get some help with this PR from the official Discord server, but discussion was pretty stale. So I'm hoping by opening this PR we can get this change going. Or, if the conclusion is 'we shouldnt do this at all', that's fine too!
Adds optional arguments for URL params
hashandsearchParamstoresolve(). Appends these to the returned URL after resolving the path.Resolves #14750 and helps address sveltejs/eslint-plugin-svelte#1353
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
kit/packages/kit/test/apps/basics/test/client.test.js, but was met withReferenceError: __SVELTEKIT_PAYLOAD__ is not defined at ../../../../src/runtime/app/paths/internal/client.js:1. This makes sense, becauseresolveRouteis used inresolve, which usesbase, which needs__SVELTEKIT_PAYLOAD__. I wasn't able to find any existing tests forresolveorresolveRoute, other than atpackages/kit/test/apps/options/test/test.jsat line97, which seems to be more of an integration test than actually testingresolveRouteitself.This is my first PR for this repo, so I'm not sure where below tests should go.
Willing to add these if I get some pointers in the right direction.
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits