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.

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

Communicate Visit direction with html[data-turbo-visit-direction] #1007

Merged
merged 9 commits into from
Dec 4, 2023
Prev Previous commit
Next Next commit
Fix typos
domchristie authored Nov 17, 2023

Verified

This commit was signed with the committer’s verified signature.
jvnipers Juniper
commit 6912e8891cbfd816843dbbe486935c120c7e7d87
2 changes: 1 addition & 1 deletion src/tests/functional/visit_tests.js
Original file line number Diff line number Diff line change
@@ -229,7 +229,7 @@ test("Visit direction data attribute when clicking a link", async ({ page }) =>
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can write these tests without relying on Promise.all:

  page.click("#same-origin-link")

  await waitUntilSelector(page, "[data-turbo-visit-direction='forward']")
  await waitUntilNoSelector(page, "[data-turbo-visit-direction]")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I tried this and I don't this it worked because the attribute is added and removed quicker than the assertions can track.

If I recall correctly, in the time it takes for page.click to resolve, the attribute it already added and removed, so waitUntilSelector(page, "[data-turbo-visit-direction='forward']") never passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also line 226-227 ensures the attribute is added and removed in the correct order

Copy link
Collaborator

Choose a reason for hiding this comment

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

@domchristie what do you think of a660c7b?

The trick is that you don't await for the page.click, so it runs in parallel with the first waitUntilSelector. Then we await for the two assertions to ensure they happen one after the other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, that works locally but seems very flaky on CI.

Copy link
Collaborator

@afcapel afcapel Nov 17, 2023

Choose a reason for hiding this comment

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

I think our best option is to use the mutation logs so we are not so dependent on the exact assertion timing. With the mutation log, even if the attribute is no longer present in the page, we can still assert that it was present at some point.

See https://github.com/hotwired/turbo/compare/2bc7c13c66b2a31c4c46c63819d4f7f052538e6f..5d4758969663e4633cf5c61eb9b2bb23cf0a77a9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think our best option is to use the mutation logs

This looks good to me 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, @domchristie if you can push those changes to your branch I'll merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afcapel I will do (I’ve just moved house, and will have a reliable internet connection next week)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you can push those changes to your branch I'll merge

@afcapel Done

})

test("test Visit direction detdata attribute when navigating back", async ({ page }) => {
test("Visit direction data attribute when navigating back", async ({ page }) => {
await page.click("#same-origin-link")
await nextEventNamed(page, "turbo:load")
await Promise.all([