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

ASP-2684: move to manifest v3 #14

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

mikerudd
Copy link

@mikerudd mikerudd commented Oct 3, 2022

manifest v2 is fast approaching end of life. We need to convert to manifest v3.

As feared we seem to have hit issues adding our script to the page. This PR is for reference and should not be merged.

@@ -25,6 +25,7 @@ module.exports = function(grunt) {
cwd: './',
src: [
'*.html',
'background.js',
Copy link
Author

Choose a reason for hiding this comment

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

part of the v3 manifest changes requires that background.js is in the project's root.

@@ -0,0 +1,4 @@
import "/js/allTenants.js";
Copy link
Author

Choose a reason for hiding this comment

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

use ES6 module syntax to import multiple script files (v3 manifest requirement).

@@ -0,0 +1,62 @@
import {getActiveTenant} from "/js/tenants.js";
Copy link
Author

Choose a reason for hiding this comment

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

This file is just js/background.js renamed, to save probably just my confusion with having background.js in the root as well now.


chrome.action.onClicked.addListener(bookmark)

async function bookmark() {
Copy link
Author

Choose a reason for hiding this comment

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

some refactoring to make this async because we now have to use the new tabs api which is async.

function chromeOrBrowser() {
return this.browser || chrome;
}
// function chromeOrBrowser() {
Copy link
Author

Choose a reason for hiding this comment

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

Can only use chrome now. When uploading the manifest it complains if this wasn't dealt with.

"16": "images/icon16.png",
"48": "images/icon48.png",
"128": "images/icon128.png",
"176": "images/icon176.png"
},
"background": {
"page": "background.html",
"persistent": false
"service_worker": "background.js",
Copy link
Author

Choose a reason for hiding this comment

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

This is probably the biggest departure. Extensions only work as service workers and not background scripts. I think this will necessitate us reworking what we have.

},
"permissions": [
"activeTab", "storage", "https://talis-public.talis.com/*"
"activeTab",
Copy link
Author

Choose a reason for hiding this comment

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

Needed to add a couple more permissions for the new apis we must use, scripting and tabs

<script type="module" src="js/allTenants.js"></script>
<script type="module" src="js/tenants.js"></script>
<script type="module" src="js/options.js"></script>
<!-- <script type="module">-->
Copy link
Author

Choose a reason for hiding this comment

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

The example from the docs is still not clear to me, but this was an attempt to get the modules/service worker approach working in the first instance.

@@ -25,6 +25,7 @@ module.exports = function(grunt) {
cwd: './',
Copy link
Author

Choose a reason for hiding this comment

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

When I added type=module to package.json when I tried to build the project via grunt I was getting errors which suggested I should change the grunt file extension to cjs when I tried that the errors went away and it seemed to build fine. Just explaining why it's changed.

@mikerudd mikerudd requested a review from rsinger October 4, 2022 11:17
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.

1 participant