-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: remove unnecessary path validation which breaks fetch with custom path base #15291
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?
Conversation
🦋 Changeset detectedLatest commit: 208b2ab 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 |
1 similar comment
0df3081 to
fe1a7a2
Compare
packages/kit/test/apps/options/source/pages/routing/link-outside-base/+page.svelte
Outdated
Show resolved
Hide resolved
fe1a7a2 to
db812b3
Compare
|
I don't think this is the correct fix. Maybe changing
if (url.origin !== event.url.origin || (paths.base && !decodeURIComponent(url.pathname).startsWith(paths.base))) { |
|
Hi @PatrickG, apologies for the quick fix attempt, I lack knowledge in sveltekit codebase but tried to file the fix. Your suggestion makes sense, I've applied it and it seems to work locally. Cheers! |
| async (r) => (result = await r.json()) | ||
| ); | ||
| }, 100); | ||
| }, 50); |
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.
Why was this reduced?
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.
The test was flaky, it was failing randomly in the CI and locally, reducing the abort signal timing helped.
Should I revert this change?
.changeset/honest-cobras-jog.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@sveltejs/kit': major | |||
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.
| '@sveltejs/kit': major | |
| '@sveltejs/kit': patch |
It should only be major for a breaking change (of a previously valid behaviour?). However, I don't think anyone would purposely try to have the load fetch trigger the SvelteKit server response if it's requesting an entirely different path
| expect(await page.locator('[data-testid="fetch-url"]').textContent()).toContain( | ||
| `${baseURL}/not-base-path/` | ||
| ); | ||
| expect(await page.locator('[data-testid="fetch-response"]').textContent()).toContain( |
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.
Passing a locator and awaiting the expect is the recommended Playwright API
| expect(await page.locator('[data-testid="fetch-url"]').textContent()).toContain( | |
| `${baseURL}/not-base-path/` | |
| ); | |
| expect(await page.locator('[data-testid="fetch-response"]').textContent()).toContain( | |
| await expect(page.locator('[data-testid="fetch-url"]')).toHaveText( | |
| `${baseURL}/not-base-path/` | |
| ); | |
| await expect(page.locator('[data-testid="fetch-response"]')).toHaveText( |
| test('fetch to root succeeds', async ({ page, baseURL }) => { | ||
| await page.goto('/path-base/fetch/link-root/'); | ||
| // fetch to root with trailing slash | ||
| expect(await page.locator('[data-testid="fetch1-url"]').textContent()).toContain(`${baseURL}/`); | ||
| const fetch1Response = await page.locator('[data-testid="fetch1-response"]').textContent(); | ||
| const fetch1Redirect = await page.locator('[data-testid="fetch1-redirect"]').textContent(); | ||
| expect( | ||
| // production | ||
| fetch1Response?.includes('did you mean to visit') || | ||
| // dev | ||
| fetch1Redirect === '/path-base' | ||
| ).toBe(true); | ||
|
|
||
| // fetch to root without trailing slash should be relative | ||
| expect(await page.locator('[data-testid="fetch2-url"]').textContent()).toBeFalsy(); | ||
| expect(await page.locator('[data-testid="fetch2-response"]').textContent()).toBe('relative'); | ||
|
|
||
| // fetch to root with custom base path with trailing slash | ||
| expect(await page.locator('[data-testid="fetch3-url"]').textContent()).toBeFalsy(); | ||
| expect(await page.locator('[data-testid="fetch3-response"]').textContent()).toBe('root'); | ||
|
|
||
| // fetch to root with custom base path without trailing slash | ||
| expect(await page.locator('[data-testid="fetch4-url"]').textContent()).toBeFalsy(); | ||
| expect(await page.locator('[data-testid="fetch4-redirect"]').textContent()).toBe('/path-base/'); | ||
| }); |
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.
I'm not sure if we need this test. It doesn't help validate that the non-base path URL is fetchable. Does this test fail before the fix is applied?
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.
no it was working before as well, I will remove the base path URL fetches 👍
teemingc
left a comment
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.
Thanks for the PR! I left a few comments
closes #11078
When having a custom base path:
And making a request to a different server hosted on the same domain,
+layout.server.ts:Making a request to the same origin but to a different server not with the configured
paths.base, currently doesn't work but this should be allowed and possible as they share the same domain name.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
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