Update downloads to mv3 #844
Conversation
|
@oliverdunk would you be able to review? |
There was a problem hiding this comment.
Thanks so much for taking a look at this!
I left some initial thoughts - in both cases the extensions aren't working right now so it would be great to fix that. Maybe you had some extra changes that you didn't commit?
It would also be great to run the linter (npm run lint:fix). Note that this will change var to let which will break the download_links extension when you open the popup more than once, since it will try to re-declare variables. We'll need to look at fixing that.
| { | ||
| "name": "Download Selected Links", | ||
| "description": "Select links on a page and download them.", | ||
| "version": "0.3", |
There was a problem hiding this comment.
Given the previous version was 0.1, this should probably be 0.2?
| "activeTab", | ||
| "scripting" | ||
| ], | ||
| "content_scripts":[ |
There was a problem hiding this comment.
Given that we inject this script from the popup, do we need a content script?
| } | ||
| } | ||
|
|
||
| console.log(links) |
There was a problem hiding this comment.
We weren't previously logging here, so I think we should probably remove this?
| "minimum_chrome_version": "16.0.884", | ||
| "permissions": [ | ||
| "downloads", | ||
| "activeTab", |
There was a problem hiding this comment.
Thanks for moving to activeTab here!
| "name": "Download Selected Links", | ||
| "description": "Select links on a page and download them.", | ||
| "version": "0.3", | ||
| "minimum_chrome_version": "16.0.884", |
There was a problem hiding this comment.
We should probably remove this, or otherwise bump it to Chrome 88 (the first version with Manifest V3 support).
|
|
||
| //using runtime sendmessage to send the links to the popup | ||
| chrome.runtime.sendMessage({ | ||
| request : links, |
There was a problem hiding this comment.
We used to just send links, rather than an object with a request property, and I think this is breaking the sample. Can you try that on your end and make any changes we need?
| @@ -0,0 +1,30 @@ | |||
| { | |||
| "name": "Downloads Manager", | |||
There was a problem hiding this comment.
We were previously using __MSG_extName__ which is the right way of doing localisation in an extension: https://developer.chrome.com/docs/extensions/mv3/i18n-messages/
Could we try and preserve that?
| "downloads", | ||
| "downloads.open", | ||
| "downloads.shelf", | ||
| "storage" |
There was a problem hiding this comment.
Is there something specific we need the storage permission for? Perhaps you were looking to use chrome.storage.local in place of localStorage?
| function maybeOpen(id) { | ||
| var openWhenComplete = []; | ||
| try { | ||
| openWhenComplete = JSON.parse(localStorage.openWhenComplete); |
There was a problem hiding this comment.
localStorage isn't available in MV3. We'll need to update this before we can test the extension.
|
Also: Removing |
|
Thanks for the feedback. On it. |
|
Does either of you have time to add READMEs to these? |
|
Yeah I will add them, once the code is ready. |
|
Hi @Shubham-Rasal! I was just wondering if you plan on continuing this one? Totally fine either way, but it would be useful so we know if we need to include this one in our work planning :) |
|
Sorry, I won't be able to work on this. You can go ahead with your plan |
No worries. Closing this for now, but we may use it as a reference when we're able to take another look at this. |
This PR aims to upgrade the existing download extension samples to MV3. Here I have migrated the downloads manager and download links samples.
If this PR is appropriate, I would love to continue to update others as well