fix: content script is incorrectly invalidated when injected multiple times#2035
fix: content script is incorrectly invalidated when injected multiple times#2035aklinker1 merged 5 commits intowxt-dev:mainfrom
Conversation
`window.postMessage` is performed asynchronously, which results in race conditions where a content script can be incorrectly invalidated `document.dispatchEvent` invokes event handlers synchronously which means only old scripts will be invalidated
No longer necessary to handle wxt-dev#884 since `document.dispatchEvent` is handled synchronously
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // Send message using `window.postMessage` for backwards compatibility to invalidate old versions before WXT changed to `document.dispatchEvent` | ||
| // TODO: Remove this once WXT version using `document.dispatchEvent` has been released for a while | ||
| window.postMessage( | ||
| { | ||
| type: ContentScriptContext.SCRIPT_STARTED_MESSAGE_TYPE, | ||
| contentScriptName: this.contentScriptName, | ||
| messageId: Math.random().toString(36).slice(2), | ||
| messageId: this.id, | ||
| }, | ||
| '*', | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think we can avoid it, and release only your solution, without this fallback
There was a problem hiding this comment.
As mentioned in #2035 (comment) and #2035 (comment), we need to keep the fallback.
|
@namuorg To be honest, i don't know why @aklinker1 worry about previous versions of scripts. Extension update on browser restart, and then all scripts and entire browser engine shut down, and then starts with updated WXT and there's no way to have older version of content script than e.g We have bug, let's merge it and solve the bug, if we were merge it e.g month ago, all the edge cases, about you were talking about, which can happend, will have a time to happend, and we won't remember about it. @namuorg That was the most professional PR, which i've seen in my live, you shown us video with repro and bug fix, that's outstanding and we shouldn't block it, because of possible one edge-case. @aklinker1 We need to go forward faster IMO, don't think too much(I don't want to say, merge without brain), but i think we need to let people know, if they'll open PR, that'll be check in e.g 1 week, maybe sth about this time, they can take any feedback(we all working, and don't have much time, to spend on open source project, but just take a look od PR, give some feedback and let people push forward. IMO this approach, can drastically improve number of PRs by @aklinker1 You have been working a looot, because i see amount of PRs, which you've merged, but i think you should do less by your own, but let people do work and only checking it. You have done a REALLY GREAT JOB, but let be some kind of DEV-Project Manager. Let me know, what are you think, maybe i'm wrong. |
|
Thanks @PatrykKuniczak, I appreciate the support! The videos took a while to set up and make so I'm glad they're helpful for understanding the PR and what it fixes. :) I understand @aklinker1's concern about not wanting to introduce breaking changes, but my understanding is that the concern is due to an inaccurate understanding of how I'm hoping that we could get this PR merged. I'm happy to address any additional questions or concerns about this change. Thanks! |
@PatrykKuniczak This is factually wrong. Content scripts do continue running after the extension is uninstalled or updated in Chrome. This is a known problem (if not well known) that extension devs need to deal with. The extension update scenario is the main reason this util exists and is so integrated with WXT - so that devs don't have to deal with having two versions of their content scripts being active at the same time. It also serves nicely as a solution during development for when the extension is reloaded or new version of the content script injected dynamically. For example, I develop an extension that changes the video player on Crunchyroll. If we don't handle this scenario, when my extension is updated while people are watching anime, the custom player would remain on the page and paritally stop working. Worse, since Crunchyroll uses SPA navigation, the page won't refresh during a binge session, compounding the errors as different episodes are played. In that case, someone may report the problem or leave a bad review when things break on them. The dev of the extension (me) would then have to spend time debugging and figuring out what happened, but it would be almost impossible find since it wasn't a code change. If they did figure out the problem came from WXT, then now it's my problem as a maintainer. So as you can see, making this breaking change would have real consequences and effect people's trust in WXT's stability. @PatrykKuniczak if you're going to be on the team, preventing breaking changes is the number one thing to look out for in PRs. I hope you understand why after this discussion.
I will make no such promises. I work on WXT in my free time and when life happens, there's no guarantee I can fulfill that commitment. I agree that we should move faster, but once again, we can't blindly merge PRs "because of one small edge case" on a critical part of the code. I was so hesitant to merge the PR at first because of how critical the
There has not been any open source contributions towards the 1.0 milestone. No one has seen fit to update dependencies on the project. People just report bugs and sometimes fix them themselves. With the current team, at least before I added you @PatrykKuniczak, I was the only (semi) active contributor. That means I have to balance security, maintainence, and feature work. I was able to merge or close some 50 PRs authored by other people in the last few months across all my OSS projects, I think that's pretty good for one person in their spare time. We just need more people on the team that I can trust to uphold the quality, stability, and vision of the framework, that's the first step to moving faster – a step I've taken with you 😄 Let's circle back on this conversation in a month after you've had more time working on the project. |
|
Anyways... sorry for the delay @namuorg! As mentioned here, my concern about
Edit: I just looked at the changes in this PR and you've already done this! Way ahead of me. Approving and merging 😄 |
@wxt-dev/analytics
@wxt-dev/auto-icons
@wxt-dev/browser
@wxt-dev/i18n
@wxt-dev/module-react
@wxt-dev/module-solid
@wxt-dev/module-svelte
@wxt-dev/module-vue
@wxt-dev/runner
@wxt-dev/storage
@wxt-dev/unocss
@wxt-dev/webextension-polyfill
wxt
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2035 +/- ##
==========================================
+ Coverage 75.99% 76.05% +0.05%
==========================================
Files 113 113
Lines 3049 3044 -5
Branches 686 685 -1
==========================================
- Hits 2317 2315 -2
+ Misses 648 645 -3
Partials 84 84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for helping make WXT better! |
|
Thanks @aklinker1! Appreciate you taking another look and all the work you've done on this project in your free time <3 |
|
@aklinker1 i'm trying me best, to understand this project, thanks for your feedback, it's have much value :) 50 PRs is very good, as one person, and i'm appreciate it But i want to bring this project alive, which i see is awaked right now :) @namuorg For next time, if you deleted a branch, you can restore it and reopen PR, by this, you can avoid duplicated PR's |
The deprecated `window.postMessage` path for the `wxt:content-script-started` message causes bad interactions with sites that have a poorly implemented event listener. This PR adds an option to exclude the `window.postMessage` event entirely, until such time as WXT completely removes it, which is a breaking change. Related PRs wxt-dev#1938 wxt-dev#2035
The deprecated `window.postMessage` path for the `wxt:content-script-started` message causes bad interactions with sites that have a poorly implemented event listener. This PR adds an option to exclude the `window.postMessage` event entirely, until such time as WXT completely removes it, which is a breaking change. Related PRs wxt-dev#1938 wxt-dev#2035
3 participants
See #1938 for the original PR and discussion (I didn't realize that deleting my fork would automatically close the existing PR for this)