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

Manifest V3 #297

Merged
merged 18 commits into from
Jul 10, 2024
Merged

Manifest V3 #297

merged 18 commits into from
Jul 10, 2024

Conversation

SidneyNemzer
Copy link
Collaborator

@SidneyNemzer SidneyNemzer commented Jun 12, 2024

This PR builds on the excellent work by @eduardoacskimlinks (#282), which is included in this PR.

  1. Removed ports, use sendMessage directly instead. This fixes a problem where the port stops working when the service worker sleeps. (by @eduardoacskimlinks)
  2. Add event listeners when the service worker starts via createWrapStore(). This addresses an issue where webext-redux wouldn't process dispatches or other messages if the message woke the service worker. This is a breaking change but I believe required for MV3. See diagram below and commit for details. bf485ad
  3. Removed the ability to connect to stores in other extensions (ie new Store({ extensionId: '...' })). See details in the commit message. 5c7d998
Diagram for (2)

Closes #284, closes #244

TODO: (rough order)

eduardoacskimlinks and others added 6 commits June 9, 2024 22:43
The goal is to use event to sync content-scripts and background-script store by doing the following steps:
1. Content-script load in the tab/page and request the latest state available to the background-script
2. Background script will broadcast any changes to the store
3. Background script will initialise with the latest state available in chrome.storage (Implemented outside of the library for the time being) and broadcast the state to all content-scripts running in Chrome
@SidneyNemzer SidneyNemzer mentioned this pull request Jun 12, 2024
wrapStore is no longer directly exported, it's constructed by createWrapStore. createWrapStore must be called synchronously when the service worker starts, allowing any message events to be queued and processed later after wrapStore is called.

This avoids dropping dispatches from extension contexts that wake the service worker with a message.
Previously, we used ports to know which external
contexts were interested in store updates. We
don't use ports anymore, so there isn't an obvious
mechanism to know which external contexts are
interested in store updates.

This means webext-redux can't be used across
extensions anymore. I'm not sure how often this
was used [1], but if there's use-cases for this,
we could consider tracking contexts that recently
requested messages and broadcast state updates to
them, in addition to broadcasting state updates to
the open tabs and our own extension pages.

1: Tried searching GitHub but found no results.
This indicates the `extensionId` option is never
used in open-source code. Or I didn't use the
right search query.

https://github.com/search?q=webext-redux+extensionId&type=code
Copy link
Contributor

@eduardoacskimlinks eduardoacskimlinks left a comment

Choose a reason for hiding this comment

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

LGTM, added some non-blocker ideas.

src/listener.js Show resolved Hide resolved

// We request the latest available state data to initialise our store
this.browserAPI.runtime.sendMessage(
{ type: FETCH_STATE_TYPE, portName }, undefined, this.initializeStore
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I was responsible for not doing it 😅, but now that it will become part of the main package, I wonder if we are drooping port, and if it would make sense to rename portName to a more accurate representation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, it would make sense to rename. The first name that comes to mind is messageType, any other ideas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ended up renaming to channelName

index.d.ts Outdated Show resolved Hide resolved
src/constants/index.js Outdated Show resolved Hide resolved

default:
// do nothing
if(message.portName === this.portName){
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: formatting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My follow-up pr #298 lints some additional files but this specific case isn't handled by the existing ESLint config. We should setup prettier in the future.

src/store/Store.js Outdated Show resolved Hide resolved
The default value is 0
chromex was likely chosen since that was part of the original name for the library: 739d7fe#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R2

Updating the prefix to webext for clarity when debugging. It's not clear if these constants are part of the public API so I'll mention this in the changelog just in case someone relies on the value.
We no longer use ports for communication.
ie messages that are sent over the messaging API but not created by this library. This commit adds some guards to avoid throwing type errors at runtime.
@SidneyNemzer SidneyNemzer merged commit 11fc660 into master Jul 10, 2024
3 checks passed
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.

Is manifest version 3 support planned? Manifest V3
3 participants