remix
remix copied to clipboard
[Bug]: App crash on fast browser navigation or shows wrong url
Reproduction case for issue: https://github.com/remix-run/remix/issues/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:
- this is a test that reproduces the behavior in issue #1757
⚠️ No Changeset found
Latest commit: edcd79329cd388d22d0be06558a6695f5a2ab2dc
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
@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 thanks for taking a look. This fails locally for me consistently, let me try to get it failing for your ci
@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. `
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.
@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.
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.
I still believe this is an issue. I will update the repro case against latest remix version
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! 🙂
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 👍
@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
@brophdawg11 PR is here https://github.com/remix-run/remix/pull/6409 (sorry for dup msgs, covering all channels)