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

fix(context): Deduplicate wxt:content-script-started #1364

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

DenizUgur
Copy link
Contributor

@DenizUgur DenizUgur commented Jan 22, 2025

This fix prevents premature context invalidation. Some pages might send the same message again, either by design or a flaw. That shouldn't interfere with the context invalidation logic.

We could go even further and add a UUID to the event in stopOldScripts and check that but timestamp might be good enough.

fixes #1359

> This fix prevents premature context invalidation
> fixes #1359
Copy link

netlify bot commented Jan 22, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 0cd4a56
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/67987a209836620009251b27
😎 Deploy Preview https://deploy-preview-1364--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@aklinker1
Copy link
Collaborator

Some pages might send the same message again, either by design or a flaw.

Not by design, I'm curious what is sending the message a second time... Is the message coming from a different frame? The same frame? I don't really understand why we need to deduplicate handling the event when we shouldn't be sending the event a second time in the first place.

@DenizUgur
Copy link
Contributor Author

When I was debugging the library, I have noticed that wxt doesn't send the event twice but rather the page I've linked would sporadically dispatch the events again. With the uglified source code it's hard to say what their justification was.

Are you able to reproduce the issue?

@aklinker1
Copy link
Collaborator

aklinker1 commented Jan 27, 2025

No, I was not. A separate issue with vite 6.0.9 prevented me from following the repro steps. Been trying to fix that. See #1359 and #1362

But ok, the website sending the event more than one makes sense, thanks for the additional info. In the case, since we can't control when the website re-sends the events or if it can change the event values, we should previously use the ID approach you mentioned instead of a timestamp.

@DenizUgur
Copy link
Contributor Author

I've revised the PR to use UUID. Last commit b099287 can be reverted if it's not necessary to validate the origin of the messages.

Copy link
Collaborator

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good now. Last change - Can we just use Math.random for the ID instead of installing a new package? Or if you want to use a UUID, the builtin crypto API can generate one for you on modern browsers:

https://developer.mozilla.org/en-US/docs/Web/API/Crypto

const id = crypto?.randomUUID?.() ?? Math.rand()

But I think a random number would be fine. Then we don't need to check if the crypto API is available.

@DenizUgur
Copy link
Contributor Author

Sure, I didn't want to use crypto.randomUUID because it's only available in secure contexts. I agree though, random number would be enough.

@aklinker1
Copy link
Collaborator

Nice, thanks. Running checks, will get this merged and deployed once they pass and I get back on

Copy link

pkg-pr-new bot commented Jan 27, 2025

Open in Stackblitz

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1364

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1364

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1364

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1364

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1364

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1364

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1364

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1364

wxt

npm i https://pkg.pr.new/wxt@1364

commit: 0cd4a56

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.21%. Comparing base (a205f2a) to head (0cd4a56).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1364      +/-   ##
==========================================
- Coverage   81.34%   81.21%   -0.14%     
==========================================
  Files         128      128              
  Lines        6288     6296       +8     
  Branches     1072     1073       +1     
==========================================
- Hits         5115     5113       -2     
- Misses       1158     1168      +10     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aklinker1 aklinker1 changed the title Deduplicate wxt:content-script-started fix(context): Deduplicate wxt:content-script-started Jan 28, 2025
@aklinker1 aklinker1 merged commit a53ee6d into wxt-dev:main Jan 28, 2025
18 checks passed
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.

Premature context invalidation for content scripts
2 participants