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

test: multiple page tests for #492 #493

Merged
merged 8 commits into from
Mar 24, 2024

Conversation

techfg
Copy link
Contributor

@techfg techfg commented Nov 5, 2023

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Tests to reproduce #492

Use cases and why

When multiple tabs/pages are open, the error $instanceId$ not found is encountered when attempting to access main thread. See #492 for details.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link

vercel bot commented Nov 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
partytown ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 24, 2024 10:57am

@gioboa
Copy link
Member

gioboa commented Jan 10, 2024

Thanks @techfg I'll resolve the conflicts.

@techfg
Copy link
Contributor Author

techfg commented Jan 10, 2024

Thanks @techfg I'll resolve the conflicts.

Thanks @gioboa! Unfortunately, the merge resulted in two tests no longer failing so I've reverted back to the order of operations that existed from prior commit. In short, the bug that exists is whenever two (or more) tabs/pages are open, if you go back to the first page (and sometimes even on the 2nd), you'll encounter the error so it's important for both pages to be loaded and then interact with them.

This does seem to be a fairly significant issue as any time more than one tab/page is open on a site that uses PT, unless Atomics are used, the problem will arise. Any insight on when #492 could be looked at?

@gioboa
Copy link
Member

gioboa commented Jan 11, 2024

May I suggest you to checking it out pls?

@techfg
Copy link
Contributor Author

techfg commented Jan 11, 2024

May I suggest you to checking it out pls?

Sorry @gioboa, is question intended for me? If so, sorry, I don't understand what you'd like me to check out?

@gioboa
Copy link
Member

gioboa commented Jan 11, 2024

Any insight on when #492 could be looked at?

I was referring to this.

@techfg
Copy link
Contributor Author

techfg commented Jan 11, 2024

Any insight on when #492 could be looked at?

I was referring to this.

@gioboa - gotcha, thanks for clarifying. I was the one that created #492. I believe the root issue is in the way PT tracks instances but doesn't recognize multiple windows when posting messages back and forth. I'm not familiar with the core internals so was hopeful someone from PT team who's more familiar could review and resolve. Steps to easily reproduce issue are in #492. Thanks!

@techfg
Copy link
Contributor Author

techfg commented Jan 16, 2024

@gioboa - Forgot to mention that when I initially found the issue and added the tests to reproduce, I did look in to what might be causing this. It was a while ago so can't recall exact details but I believe I found that when the 2nd (or any subsequent) window opens and PT loads, the information (e.g., instances) are dropped from the internal tracking mechanism and only the new window's instances remain which leads to the $instanceId$ not found errors.

@gioboa gioboa merged commit 4a48810 into QwikDev:main Mar 24, 2024
7 of 8 checks passed
@techfg techfg deleted the add-multiple-pages-tests branch March 24, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants