-
Notifications
You must be signed in to change notification settings - Fork 436
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
Lift the frame morphing logic up to FrameController.reload #1192
Changes from 18 commits
c03c893
047a479
e1fde68
5427b77
9fd1e9d
a5a87e4
87c9553
07c3577
699f66f
6abb120
38e4d2c
8344e4d
a342115
1ed1dc1
f738bdb
e956dd4
fceb2cb
3af219d
a0bf9a3
f45697c
55266c7
8f0a949
6801493
795ab37
61bd384
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,7 +160,7 @@ test("doesn't morph when the navigation doesn't go to the same URL", async ({ pa | |
expect(await noNextEventNamed(page, "turbo:render", { renderMethod: "morph" })).toBeTruthy() | ||
}) | ||
|
||
test("uses morphing to update remote frames marked with refresh='morph'", async ({ page }) => { | ||
test("uses morphing to only update remote frames marked with refresh='morph'", async ({ page }) => { | ||
await page.goto("/src/tests/fixtures/page_refresh.html") | ||
|
||
await page.click("#form-submit") | ||
|
@@ -180,12 +180,15 @@ test("uses morphing to update remote frames marked with refresh='morph'", async | |
test("don't refresh frames contained in [data-turbo-permanent] elements", async ({ page }) => { | ||
await page.goto("/src/tests/fixtures/page_refresh.html") | ||
|
||
// Set the frame's text since the final assertion cannot be noNextEventOnTarget as that is passing even when the frame reloads. | ||
const frame = page.locator("#remote-permanent-frame") | ||
await frame.evaluate((frame) => frame.textContent = "Frame to be preserved") | ||
|
||
await page.click("#form-submit") | ||
await nextEventNamed(page, "turbo:render", { renderMethod: "morph" }) | ||
await nextBeat() | ||
|
||
// Only the frame marked with refresh="morph" uses morphing | ||
expect(await noNextEventOnTarget(page, "refresh-reload", "turbo:before-frame-morph")).toBeTruthy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion was not working. When I commented out code and convinced myself the permanent frame WAS reloading, this test would still pass. I think it's a race condition where the assertion breezes through too quickly. I tried multiple different strategies to make this test durable and the solution you see here is the only one that worked. |
||
await expect(page.locator("#remote-permanent-frame")).toHaveText("Frame to be preserved") | ||
}) | ||
|
||
test("frames marked with refresh='morph' are excluded from full page morphing", async ({ page }) => { | ||
|
@@ -201,7 +204,7 @@ test("frames marked with refresh='morph' are excluded from full page morphing", | |
await expect(page.locator("#refresh-morph")).toHaveText("Loaded morphed frame") | ||
}) | ||
|
||
test("navigated frames without refresh attribute are reset after morphing", async ({ page }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry if I explained wrong. Turbo frames that aren't reloaded by morphing have to be reset correctly. This test should be fine. Turbo frames that don't have the conditions to be reloaded with morphing are affected in the first round of morphing, therefore, they are morphed according to the changes coming from the server. Turbo frames that can be reloaded with morphing due to the presence of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just about to send a long final comment saying I finished. :) Happy to do this either way. This test is a bit confusing so it took me a bit to fully make sense of. In your longer explanation, you said what my expectation would be. Specifically:
This test was enforcing the opposite, so I flipped it to reflect what you said here. ^ And it fits my intuition that if a user navigations a turbo frame, and it's implicitly reload="refresh" (not morph) then I would expect that navigation to stay. But you think we should revert it back since it started with no src when the page loaded? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct, but only for Turbo frames marked with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is that the goal of the test as it was before was correct. Turbo frames that don't contain the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test has been reverted (so that it fails again) and the logic has been corrected (so now the test passes). I think that's it and things are now ready. 🤞 |
||
test("navigated frames without refresh attribute are not reset after morphing", async ({ page }) => { | ||
await page.goto("/src/tests/fixtures/page_refresh.html") | ||
|
||
await page.click("#refresh-after-navigation-link") | ||
|
@@ -213,17 +216,20 @@ test("navigated frames without refresh attribute are reset after morphing", asyn | |
"navigates theframe" | ||
) | ||
|
||
//await new Promise(resolve => setTimeout(resolve, 300000)) | ||
|
||
await page.click("#form-submit") | ||
|
||
await nextEventNamed(page, "turbo:render", { renderMethod: "morph" }) | ||
await nextBeat() | ||
|
||
assert.ok( | ||
|
||
assert.notOk( | ||
await hasSelector(page, "#refresh-after-navigation-link"), | ||
"resets the frame" | ||
) | ||
|
||
assert.notOk( | ||
assert.ok( | ||
await hasSelector(page, "#refresh-after-navigation-content"), | ||
"does not reload the frame" | ||
) | ||
|
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.
Every other test was referencing
click("#hello a")
so I updated this one for consistency.