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

[Bug]: App crash on fast browser navigation or shows wrong url #5166

Closed
wants to merge 7 commits into from

Conversation

n8agrin
Copy link
Contributor

@n8agrin n8agrin commented Jan 20, 2023

Reproduction case for issue: #1757

To run:

yarn bug-report-test

Failure is either an error where javascript complains about default missing from an object, or a url not matching its content. In the case of the test, we expect /burgers to contain the string cheeseburgers, but often only gets to the first route in history, /

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Jan 20, 2023

⚠️ No Changeset found

Latest commit: edcd793

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@machour machour changed the title Add repro case for issue 1757 [Bug]: App crash on fast browser navigation Jan 22, 2023
@machour
Copy link
Collaborator

machour commented Jan 22, 2023

@n8agrin this PR seems to fail at reproducing the issue, can you check why?
Also you may want to use a for loop to help readability 🙏

@machour machour added the needs-response We need a response from the original author about this issue/PR label Jan 22, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jan 22, 2023
@n8agrin n8agrin mentioned this pull request Jan 22, 2023
2 tasks
@n8agrin
Copy link
Contributor Author

n8agrin commented Jan 22, 2023

@machour thanks for taking a look. This fails locally for me consistently, let me try to get it failing for your ci

@n8agrin n8agrin changed the title [Bug]: App crash on fast browser navigation [Bug]: App crash on fast browser navigation or shows wrong url Jan 23, 2023
@n8agrin
Copy link
Contributor Author

n8agrin commented Jan 23, 2023

@machour getting this to fail on CI was tricky. I have not been able to reproduce the case where javascript throws an exception, complaining TypeError: Cannot destructure property 'default' of 'n[e]' as it is undefined even though I can get this to error locally using this test every time.

The bug requires transitioning from a url that is not the remix app under test, to a url in a remix app which fails to load the correct module for the page. This is typically done either by browsing back and forth quickly, or finding a rare state, where Remix issues a 300 level http redirect to a module that has not loaded yet. This is why I've added a navigation event to https://remix.run itself, to ensure the test clears the remix app under test's modules from memory.

To get this to fail on CI I took a different tact, which leverages another issue I've observed locally: when this error manifests, the url does not match the page content. This seems to fail consistently on ci. That said, occasionally the flaky test retries were able to overcome the particular check, requiring I add some retries in order to force the failure.

The behavior this test is trying to assert is flaky by nature. It's difficult to capture it in a succinct regression test. I recommend checking out this branch locally and running the tests for yourself. If you'd like to see the TypeError exception, follow the directions I've left in the code comments and you should trivially reproduce the issue.

Also happy to jump on a call with you to explain it.

This error is blocking our production app from being able to be tested in CI, causing us and our customers real pain.
`

@machour
Copy link
Collaborator

machour commented Jan 23, 2023

Thank you so much for all the efforts trying to reproduce this issue @n8agrin.
Yes, this is definitely not easy to reproduce in CI.

I'm going to defer to @mcansh (our testing expert) or @brophdawg11 (our routing expert) on this one.
Nothing much I can do on my side.

@n8agrin
Copy link
Contributor Author

n8agrin commented Jan 23, 2023

@machour thank you! @mcansh @brophdawg11 I've separated the failure cases into two tests. Hopefully you are able to reproduce locally and see the errors described above. Please feel free to reach out and let me know how I can help.

@github-actions
Copy link
Contributor

This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed.

@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Apr 18, 2023
@n8agrin
Copy link
Contributor Author

n8agrin commented Apr 19, 2023

I still believe this is an issue. I will update the repro case against latest remix version

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 19, 2023
@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 2, 2023
@github-actions
Copy link
Contributor

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this May 12, 2023
@brophdawg11
Copy link
Contributor

I was about to re-open since this is not yet fixed but since we the issue is open, I think it's OK for this to remain closed since we can still see this reproduction when we get to addressing that bug 👍

@n8agrin
Copy link
Contributor Author

n8agrin commented May 15, 2023

@brophdawg11 coincidentally, yesterday I worked out a patch that fixes the underlying bug. I’ll post the diff tonight when I’m back at my keyboard

@n8agrin
Copy link
Contributor Author

n8agrin commented May 16, 2023

@brophdawg11 PR is here #6409 (sorry for dup msgs, covering all channels)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed needs-response We need a response from the original author about this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants