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

Transition from manifest v2 to v3 #1367

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Transition from manifest v2 to v3 #1367

wants to merge 15 commits into from

Conversation

TarikGul
Copy link
Member

closes: #1316
closes: #310

[WIP]

This PR has a lot left to fix, and test between firefox and chrome. A more thorough deccription will be provided as I work through all the errors that are present in the build.

@TarikGul
Copy link
Member Author

So far the extension is working fine in chrome with the exception of the following errors:

Error: Invalid url chrome://extensions/?errors=fnpncejekijbiiemffncneiiilejbmbj, expected to start with http: or https: or ipfs: or ipns:

@TarikGul TarikGul requested a review from Tbaut May 30, 2024 17:15
@TarikGul
Copy link
Member Author

TarikGul commented May 30, 2024

Instead of causing breaking changes to extension-base by enforcing async at the top level of State, I am going to just use the callback method for storage.local

Edit: Ahh but this might not work since there is no more persistent background.

@Tbaut
Copy link
Contributor

Tbaut commented May 31, 2024

edit: There was a stray change in the current state to avoid a TypeError: chrome.extension.getURL is not a function in packages/extension-base/src/background/handlers/State.ts

- const NOTIFICATION_URL = chrome.extension.getURL('notification.html');
+ const NOTIFICATION_URL = chrome.runtime.getURL('notification.html');

I had a quick look on Firefox, changing the manifest to make it happy with what I found here which I'm sure you know

  "background": {
    "scripts": ["background.js"]
  },

edit2: In Firefox the extension is then loaded correctly, I could add an account, but no script is injected on pages, so Dapps don't detect the extensions.

On chrome/brave, the next error I encountered in background.js was

ReferenceError: localStorage is not defined

@TarikGul
Copy link
Member Author

@Tbaut Yea the I was able to fix the localStorage error but then reverted my changes because I would like to change the design of the implementation, but yea the localStorage issue will be fixed soon :)

@adamdossa
Copy link

Is it expected that this PR will be merged in the near future? We have a downstream dependency for our Polymesh Wallet which requires this in order to move to manifest V3. @Tbaut

@TarikGul
Copy link
Member Author

TarikGul commented Jul 2, 2024

Is it expected that this PR will be merged in the near future? We have a downstream dependency for our Polymesh Wallet which requires this in order to move to manifest V3. @Tbaut

Yes this is absolutely a priority! My goal is to get it done this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants