[fix] browser only redirect during load #2462

baseballyama · 2021-09-20T10:40:24Z

I simply implemented it according to the comments of #2424.
And I think it works well.
But I'd like to know if there's anything I haven't considered.

Question

If there are multiple redirects, what should I do?
redirect: (redirect && redirect.loaded) ? redirect.loaded.redirect : undefined,

=>
I think mainly this is a programming mistake.
So it doesn't need to handle it.
But it might be kind to throw an error if multiple redirects are there.

Thank you!

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

changeset-bot · 2021-09-20T10:40:27Z

🦋 Changeset detected

Latest commit: 8c299ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

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/runtime/client/renderer.js

benmccann · 2021-09-20T15:30:51Z

packages/kit/test/apps/basics/src/routes/redirect-on-load/_tests.js

+
+/** @type {import('test').TestMaker} */
+export default function (test, is_dev) {
+	test('redirect-on-load', '/redirect-on-load', async ({ base, page, clicknav }) => {


I don't think this is testing the new functionality you added. I think it's testing server-side redirect. If you look at some of the other tests there should be examples of testing both

Oops. sorry!
I updated!
(This time, again I checked tests that fails without this PR but passes with it.)

And during the update, I faced puppeteer issue due to redirect.
So I edited packages/kit/test/test.js in the last commit.

benmccann · 2021-09-22T20:34:08Z

packages/kit/test/apps/basics/src/routes/redirect-on-load/_tests.js

+export default function (test, is_dev) {
+	test('redirect-on-load', '/redirect-on-load', async ({ base, page, js }) => {
+		if (js) {
+			await page.waitForTimeout(50);


I would not use this. Per the docs for that method:

Note that page.waitForTimeout() should only be used for debugging. Tests using the timer in production are going to be flaky. Use signals such as network events, selectors becoming visible and others instead.

Yes. Totally agree with you.
But already few exists tests use waitForTimeout so I followed that.

Then at least I will update this PR.
But should I refactor exists waitForTimeout?
I think we should do it to avoid random test failures.
If yes, I will create one more PR which is for existing tests with waitForTimeout.

Yes, that would be great to fix the other tests so that they don't use waitForTimeout. Thank you! I filed an issue to track it: #2473

Oh, Thank you for creating a new issue. I will handle it.

I have added a function to avoid the use of waitForTimeout.
I hope to get your review.

And I will create a new PR for removing all waitForTimeout based on this branch.

benmccann · 2021-09-24T21:58:46Z

What is the new test code for get_elements / iframe? It doesn't look related to redirects, so I'm not sure why it's included in this PR

baseballyama · 2021-09-25T02:15:26Z

What is the new test code for get_elements / iframe? It doesn't look related to redirects, so I'm not sure why it's included in this PR

Thank you for your review!

I want to write a test for get_elements because this is a new code,
But I didn't find proper place, so I added it into routes folder.

I can understand this is a little bit weird.

Then I have 3 opinions and I prefer No3 but what do you think?

Write test in route folder (My current way)
--> This is reasonable to write test, but this is not test for Kit's spec. So it's little bit weird.
create new test file which is like test.spec.js and write test for get_elements.
--> It relatively takes time and effort.
No test
--> get_elements will not update frequently and already I checked that it works by existing test cases. So there is no risk even there is no test.

benmccann · 2021-09-25T03:46:27Z

I don't think we should need to add a get_elements method. We just need to check that we loaded the URL which was redirected to

baseballyama · 2021-09-25T07:36:14Z

I don't think we should need to add a get_elements method. We just need to check that we loaded the URL which was redirected to

Hum....
I re-checked that the waiting process like waitForTimeout or get_elements was necessary or not.
But now we don't need it. (I'm a bit confused because I remember checking this out yesterday, and thought I needed it.)
(Could it be dependent on CPU usage or some machine resource??)
(But I run pnpm:test many times but always passed.)

Anyway now this PR doesn't need any waiting process except redirect process.

And at next PR (#2488),
I want to solve this your comment.

What is the new test code for get_elements / iframe? It doesn't look related to redirects, so I'm not sure why it's included in this PR

I hope I will get your opinion about it.
(Personally, I prefer No3)

No test

packages/kit/test/apps/basics/src/routes/redirect-on-load/_tests.js

benmccann · 2021-09-27T15:53:37Z

packages/kit/test/test.js

+
+					// 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.


Suggested change

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

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

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

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

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?

benmccann · 2021-09-27T16:03:29Z

packages/kit/test/test.js

+							break;
+						} catch (e) {
+							if (!(e instanceof Error) || i === max) throw e;
+							await new Promise((resolve) => setTimeout(resolve, 10));


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

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.)

packages/kit/test/test.js

benmccann · 2021-09-28T15:14:14Z

thank you!!

baseballyama added 3 commits September 20, 2021 19:25

fix browser-only redirect

f6109b4

add tests

5621908

add changeset

e1f6b15

dummdidumm reviewed Sep 20, 2021

View reviewed changes

packages/kit/src/runtime/client/renderer.js Outdated Show resolved Hide resolved

baseballyama added 2 commits September 20, 2021 21:39

stop to use optional chainning

b99330a

fix lint

d3eb920

This comment has been minimized.

Sign in to view

benmccann reviewed Sep 20, 2021

View reviewed changes

update tests

fe6db2e

benmccann reviewed Sep 22, 2021

View reviewed changes

baseballyama force-pushed the fix/2424-fix-browser-only-redirect branch 2 times, most recently from a160240 to c218eb6 Compare September 24, 2021 17:24

baseballyama mentioned this pull request Sep 24, 2021

[chore] Remove usages of waitForTimeout #2488

Merged

5 tasks

wait for redirect without compromising performance.

4df87fd

baseballyama force-pushed the fix/2424-fix-browser-only-redirect branch from c218eb6 to 4df87fd Compare September 25, 2021 07:16

refactor

cac6cf2

benmccann reviewed Sep 25, 2021

View reviewed changes

packages/kit/test/apps/basics/src/routes/redirect-on-load/_tests.js Outdated Show resolved Hide resolved

remove unused args

d4c971d

benmccann reviewed Sep 27, 2021

View reviewed changes

baseballyama added 2 commits September 28, 2021 19:39

fix typo

47634cf

refactor: stop to use .

085967c

baseballyama mentioned this pull request Sep 28, 2021

Lint config improvement suggestion sveltejs/svelte#6782

Closed

benmccann reviewed Sep 28, 2021

View reviewed changes

packages/kit/test/test.js Show resolved Hide resolved

Update packages/kit/test/test.js

8c299ca

benmccann merged commit b61acee into sveltejs:master Sep 28, 2021

github-actions bot mentioned this pull request Sep 28, 2021

Version Packages (next) #2516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] browser only redirect during load #2462

[fix] browser only redirect during load #2462

baseballyama commented Sep 20, 2021 •

edited

Loading

changeset-bot bot commented Sep 20, 2021 •

edited

Loading

This comment has been minimized.

benmccann Sep 20, 2021

baseballyama Sep 21, 2021

benmccann Sep 22, 2021

baseballyama Sep 23, 2021

benmccann Sep 23, 2021

baseballyama Sep 23, 2021

baseballyama Sep 24, 2021

benmccann commented Sep 24, 2021

baseballyama commented Sep 25, 2021

benmccann commented Sep 25, 2021

baseballyama commented Sep 25, 2021

benmccann Sep 27, 2021

benmccann Sep 27, 2021

benmccann Sep 27, 2021

baseballyama Sep 28, 2021

benmccann Sep 27, 2021

baseballyama Sep 28, 2021

benmccann commented Sep 28, 2021

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

[fix] browser only redirect during load #2462

[fix] browser only redirect during load #2462

Conversation

baseballyama commented Sep 20, 2021 • edited Loading

Before submitting the PR, please make sure you do the following

Tests

Changesets

changeset-bot bot commented Sep 20, 2021 • edited Loading

🦋 Changeset detected

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Sep 24, 2021

baseballyama commented Sep 25, 2021

benmccann commented Sep 25, 2021

baseballyama commented Sep 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Sep 28, 2021

baseballyama commented Sep 20, 2021 •

edited

Loading

changeset-bot bot commented Sep 20, 2021 •

edited

Loading