fix: set trailing slash on data request 🛤 #9738
Conversation
🦋 Changeset detectedLatest commit: 5c63a9c 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 |
benmccann
changed the title
| @@ -962,6 +962,16 @@ test.describe('Routing', () => { | |||
| expect(await page.textContent('h1')).toBe('symlinked'); | |||
| }); | |||
| } | |||
| test('__data.json - trailing slash always', async ({ request }) => { | |||
There was a problem hiding this comment.
This isn't testing trailingSlash: 'always': https://github.com/sveltejs/kit/blob/99891b80238ed4cf107012480ce6a473949bc28a/packages/kit/test/apps/basics/svelte.config.js. If you intend it to then the test needs to live in a different test project. If you don't then we can rename.
the other tests all have a blank line before them. let's keep that up for consistency. also I think the test name doesn't quite follow the pattern of the others
| test('__data.json - trailing slash always', async ({ request }) => { | |
| test('can load __data.json when trailing slash is present', async ({ request }) => { |
There was a problem hiding this comment.
- line break is corrected
- added a better decription to the test
- the trailing slash config can be found here: packages/kit/test/apps/basics/src/routes/routing/trailing-slash-ssr/always/+page.js
The thing I'm trying to test, is the correct trailing slash in the pathname in __data.json, as it currently always falls back to never. (see #9595 (comment))
| const data = await r.json(); | ||
| expect(data.nodes[1].data[1]).toBe('/routing/trailing-slash-ssr/always/'); | ||
| }); | ||
| test('__data.json - trailing slash never', async ({ request }) => { |
There was a problem hiding this comment.
| test('__data.json - trailing slash never', async ({ request }) => { | |
| test('can load __data.json when trailing slash is absent', async ({ request }) => { |
There was a problem hiding this comment.
Has a better name now
.changeset/kind-clocks-exercise.md
Outdated
| '@sveltejs/kit': patch | ||
| --- | ||
|
|
||
| fix: trailing slash on \_\_data.json request |
There was a problem hiding this comment.
| fix: trailing slash on \_\_data.json request | |
| fix: compute trailing slash for data requests |
There was a problem hiding this comment.
added a better description for changeset
| test('trailing slash is correct in url.pathname in __data.json (always)', async ({ request }) => { | ||
| const r = await request.get('/routing/trailing-slash-ssr/always/__data.json'); | ||
| const data = await r.json(); | ||
| expect(data.nodes[1].data[1]).toBe('/routing/trailing-slash-ssr/always/'); |
There was a problem hiding this comment.
This relies on the internal structure of the __data.json requests. Can we instead do a more high-level test which is:
- start on
trailing-slash-ssr(btw I would rename this totrailing-slash-server) - use link clicks to go to different page with trailingSlash config
- test that the element that prints
$page.url.pathname/ returneddata.pathnamehave the desired value
Basically similar to the current routing/trailing-slash tests
There was a problem hiding this comment.
- changed folder name
- used click navigation
- updated tests to check for values in elements
I hope the changes reflect what you imagined 😊
fixes #9595
Trailing slash was never set for preloading data through
someroute/__data.jsonPlease 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:.