-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Feat/upgrade to manifest v3 #129
Conversation
Related to #120 |
@coolsoftwaretyler I don’t remember the exact logic, but I’d try to
let me know if it doesn’t help, I’ll try to take a look later |
Perfect, I think this is a great place to start. I mostly want to see the existing message passing, so I think even using connection debugging with the existing extension will help me figure out more of what needs to change. Will also peek at react-devtools migration. Will check through the week/next weekend and let you know if I need anything else. I appreciate your time! |
Ok so for some reason, setting When I open it up, I get past the I think this means that we aren't firing I took a look at the React devtools and most of the changes they made make sense to me, but don't answer the question of what has changed/broken in this extension. I'll keep tracing the message passing around through the week. I think the main problem I have is that I don't understand enough of how the existing extension is intended to work, so I don't have the right instincts about what might be broken/not communicating. Here's a video of what's happening on my end from this branch (I disabled the on-screen dev tools so I could reduce some noise in my console logs. But for what it's worth, embedding the in-app tools is working as expected) Screen.Recording.2024-10-28.at.7.21.03.PM.mp4 |
I would also try to see logs in the background page (in the window that opens if you go to chrome://extensions and click “inspect views: background page”). |
Ah yeah, I did not show that because there were no logs in prior test runs. Checked again and still nothing. I will continue debugging as well! |
Ok, in f29824f, I added some logs to Gonna drop a breakpoint there and see what's up. |
Yeehaw, I got it to work in c9f6338. I was missing the There are a ton of console logs in here I need to clean up, and I am sure there's some number of unnecessary changes. Now that I have a working commit, I am going to clean up the PR and try to pare it down to only the necessary changes. Thanks for your help so far, @andykog! Check it out! Screen.Recording.2024-10-29.at.9.05.32.PM.mp4 |
Woo-hoo 🎉 |
@coolsoftwaretyler we are almost done with "2024 Dev Readiness" 😄 |
Hey @andykog - I've cleaned up the codebase here and as far as I can tell, the remaining changes are important for actually converting over to manifest v3. There are some follow up items I'd like to do, but my goal is to merge this PR and have you ship it to the Chrome store so people can keep using the extension, even if it's still got some bugs here and there. You can test this by:
If that plan works for you, let me know when you merge and ship it, and I can close out #120 and start working on other bug reports/finding other people to help out from a starting point of a v3 extension. I've been asking around and trying to find other interested folks to jump in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I would check if it works both as a panel (in chrome devtools) and a standalone window (when clicking on the extension icon), also edge cases like reconnecting after navigating to a different site and back to the site with mobx, reloading devtools
setTimeout(() => { | ||
const newPort = chrome.runtime.connect({ name: 'content-script' }); | ||
// Update port reference | ||
port = newPort; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't we also listen for onDisconnect on this newPort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe! I am not 100% sure how all of this is wired up. What should we be doing on port disconnection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I just see that you try reconnecting after the first disconnection here, thought it would make sense to do the same for the next disconnections
For:
I think this is working! Opened up devtools, saw some MobX changes, navigated away, devtools wiped itself, and then navigated back and saw the changes again. As for opening in a separate window, there are some more deprecated APIs we have to replace to make that work. What do you think about merging this PR, and then maybe handling that as a separate PR? |
Sounds good 👍 |
const script = document.createElement('script'); | ||
script.textContent = `;(${installGlobalHook.toString()}(window))`; | ||
document.documentElement.appendChild(script); | ||
script.parentNode.removeChild(script); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change causes webpack to tree shake the hook from the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you seeing it fail to load? I tested this a bunch and things were working fine without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I'm not seeing the mobx tab in dev tools using the build on chrome web store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super weird that it worked for some of us but not in web. Maybe the production build stripped it out and dev build didn't, although I thought I ran prod builds. It's been a few weeks.
Should be an easy fix, I think if we just export something here then Webpack will keep it in.
I'll try to reproduce and fix sometime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, something weird must have happened where the old hook injection got cached in my browser or something. I was able to reproduce the bug, and see it fixed in #133
Sorry for the inconvenience!
This is a WIP branch to try and update to manifest v3.
Two big changes we have to make:
And throughout the codebase, we'll need to update some API usage.
See Google v3 checklist here: https://developer.chrome.com/docs/extensions/develop/migrate/checklist
@andykog - I've made some progress here. I updated the manifest format, removed some of the unsafe inline scripts. Now I'm stuck on getting the content script to actually connect to the backend. If you're available, I could use your help.
Mostly I'm trying to trace through all of the logic of the v2 extension. But if you have it mapped out in your head (or written down somewhere), that might help me figure out what we need to change in order to be compliant with v3.