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.

Sign up for GitHub

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

[fix] browser only redirect during load #2462

Merged
merged 12 commits into from
Sep 28, 2021
Merged

[fix] browser only redirect during load #2462

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f6109b4
fix browser-only redirect
baseballyama Sep 20, 2021
5621908
add tests
baseballyama Sep 20, 2021
e1f6b15
add changeset
baseballyama Sep 20, 2021
b99330a
stop to use optional chainning
baseballyama Sep 20, 2021
d3eb920
fix lint
baseballyama Sep 20, 2021
fe6db2e
update tests
baseballyama Sep 21, 2021
4df87fd
wait for redirect without compromising performance.
baseballyama Sep 21, 2021
cac6cf2
refactor
baseballyama Sep 24, 2021
d4c971d
remove unused args
baseballyama Sep 25, 2021
47634cf
fix typo
baseballyama Sep 28, 2021
085967c
refactor: stop to use .
baseballyama Sep 28, 2021
8c299ca
Update packages/kit/test/test.js
benmccann Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sharp-moles-hammer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix browser-only redirect during load.
2 changes: 2 additions & 0 deletions packages/kit/src/runtime/client/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,11 @@ export class Renderer {
*/
async _get_navigation_result_from_branch({ page, branch }) {
const filtered = /** @type {import('./types').BranchNode[] } */ (branch.filter(Boolean));
const redirect = filtered.find((f) => f.loaded && f.loaded.redirect);

/** @type {import('./types').NavigationResult} */
const result = {
redirect: redirect && redirect.loaded ? redirect.loaded.redirect : undefined,
state: {
page,
branch,
Expand Down
13 changes: 13 additions & 0 deletions packages/kit/test/apps/basics/src/routes/redirect-on-load/_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as assert from 'uvu/assert';

/** @type {import('test').TestMaker} */
export default function (test, is_dev) {
benmccann marked this conversation as resolved.
Show resolved Hide resolved
test('redirect-on-load', '/redirect-on-load', async ({ base, page, js }) => {
if (js) {
assert.equal(page.url(), `${base}/redirect-on-load/redirected`);
assert.equal(await page.textContent('h1'), 'Hazaa!');
} else {
assert.equal(page.url(), `${base}/redirect-on-load`);
}
});
}
17 changes: 17 additions & 0 deletions packages/kit/test/apps/basics/src/routes/redirect-on-load/index.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<script context="module">
import { browser } from '$app/env';

export async function load() {
if (browser) {
return {
status: 303,
redirect: '/redirect-on-load/redirected'
};
}

return {};
}
</script>

<h1>Woops!</h1>
<p>You shouldn't be here. You should have been directed to /redirect!</p>
1 change: 1 addition & 0 deletions packages/kit/test/apps/basics/src/routes/redirect-on-load/redirected.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<h1>Hazaa!</h1>
15 changes: 14 additions & 1 deletion packages/kit/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,20 @@ function duplicate(test_fn, config, is_build) {

if (start) {
response = await context.pages.js.goto(context.base + start);
await context.pages.js.evaluate(() => window.started);

// If redirecting, sometimes below error comes. Therefore wait until redirect.
// "page.evaluate: Execution context was destroyed, most likely because of a navigation."
// MOTE: waitForNavigation is extremely slow so doesn't use.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// MOTE: waitForNavigation is extremely slow so doesn't use.
// NOTE: waitForNavigation is extremely slow so doesn't use.

Copy link
Member

Choose a reason for hiding this comment

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

although I don't think this NOTE makes sense and I'd probably just remove it. goto on the previous line waits for the page load event, which is the same thing waitForNavigation would do. So it can't be any slower. Maybe the issue is that it's already been awaited for, but then the issue is waiting twice for the same event and not that the method is too slow

I think I'd just remove this line since it adds more confusion than it helps with

Copy link
Member

Choose a reason for hiding this comment

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

Although now I'm confused because I don't know exactly where you were going to call waitForNavigation. The other option might be to move that comment to be right above the line where you might use it

Copy link
Member Author

Choose a reason for hiding this comment

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

But In most cases, the test failed due to the bellow error without this change.

.svelte-kit/output/client/service-worker.js   9.25 KiB / brotli: 2.08 KiB
• • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • ✘ • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • • •   (232 / 233)

   FAIL  build:basics  "redirect-on-load [js]"
    page.evaluate: Execution context was destroyed, most likely because of a navigation.

    at Object.handler (file:///C:/Users/baseballyama/Desktop/git/svelte-contributes/kit-2424/kit/packages/kit/test/test.js:201:29)
    at async Number.runner (file:///C:/Users/baseballyama/Desktop/git/svelte-contributes/kit-2424/kit/node_modules/.pnpm/uvu@0.5.1/node_modules/uvu/dist/index.mjs:77:5)
    at async Module.exec (file:///C:/Users/baseballyama/Desktop/git/svelte-contributes/kit-2424/kit/node_modules/.pnpm/uvu@0.5.1/node_modules/uvu/dist/index.mjs:131:33)
    at async main (file:///C:/Users/baseballyama/Desktop/git/svelte-contributes/kit-2424/kit/packages/kit/test/test.js:394:2)

I think this is similar to microsoft/playwright#4192.

And I can't move this waiting process to redirect-on-load test file.
Because this error comes before called callback function.

But I have modified the code in response to the comment below that you gave me.
Could you please check the updated code?

const max = 100;
for (let i = 0; i <= max; i++) {
try {
await context.pages.js.evaluate(() => window.started);
break;
} catch (e) {
if (!(e instanceof Error) || i === max) throw e;
await new Promise((resolve) => setTimeout(resolve, 10));
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand in the NOTE above where you were suggesting the waitForNavigation might be called. I think that this would be a good place to do it though as an alternative to calling setTimeout. I'd prefer not to call setTimeout if possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah Definitely! I should update it!

[This is just additional explanation]

If error comes during await context.pages.js.evaluate(() => window.started);,
it means already navigation was changed.
So after call await context.pages.js.waitForNavigation();,
await context.pages.js.evaluate(() => window.started); should be succeed.
(But run a for loop in case of multiple redirects.)

}
}
}

await callback({
Expand Down