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

Set world property directly in manifest.json to skip extra permission #1013

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flexchar
Copy link

@flexchar flexchar commented Jun 23, 2024

Details

This PR disables loading Content Scripts, where world: 'MAIN' is set, using background script. The previous approach causes extra permission in to be added, which negatively impacts Chrome Store reviews and provides arguably a more scary warning popup (permission popup on install) to end users than what is necessary.

However, it keeps the existing behaviour for development to take advantage of automatically triggered browser refreshes.

This PR passes on the world property directly to the manifest. It is officially supported by the chrome. https://developer.chrome.com/docs/extensions/reference/api/scripting#type-ExecutionWorld

It is however not supported by Firefox but that doesn't alter the current behaviour (it is not supported at all).
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/scripting/ExecutionWorld#browser_compatibility

This is also the answer to my question here #433 (comment).

Legacy discussion for the initial implementation: #422

Code of Conduct

  • I agree to follow this project's Code of Conduct
  • I agree to license this contribution under the MIT LICENSE
  • I checked the current PR for duplication.

Contacts

  • (OPTIONAL) Discord ID:

If your PR is accepted, we will award you with the Contributor role on Discord server.

To join the server, visit: https://www.plasmo.com/s/d

@flexchar
Copy link
Author

I had some quirks running updated code. I got it to work but I would like extra pairs of eyes as I am not familiar with the original code base as well. :)

Comment on lines +411 to +414
//!! I am not sure what this actually does??
// metadata?.config?.world === "MAIN" && isDev
// ? scriptPath.split(scriptExt)[0]
// : scriptPath
Copy link
Contributor

Choose a reason for hiding this comment

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

This ensure we add those main world scripts into the CSP, but with the correct extension :-?

Copy link
Author

Choose a reason for hiding this comment

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

Aa, I never had problems with that so I didn't realize. Thanks!

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Let's make sure all the relevant changes are isolated? I think this and #1012 has code from each other.

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.

None yet

2 participants