Closed eduardoacskimlinks closed 1 month ago
Following the great example on https://github.com/tshaddix/webext-redux/pull/283#issuecomment-1144279331, I want to share we went live last week after several weeks of QA.
Our extension is https://chrome.google.com/webstore/detail/skimlinks-editor-tool/gmfcchdbgideoiehadmlmpnbhdcjoaih?hl=en
We release the PR using the following package https://www.npmjs.com/package/@eduardoac-skimlinks/webext-redux
Hi, @eduardoacskimlinks thanks for building it to support MV3! I used this PR and found that I get store from Popup but the state of Popup didn't sync with the state of the background script.
Let's say, I made a React component and dispatch a thunk action to the background script to fetch API, but after the background script received the API response and updated the redux state, the state of Popup was not updated. I am sure the previous webext-redux
works correctly. This issue only exists in this PR. Do you have any idea? Anyway, thanks for this nice patch again!
@eduardoacskimlinks Thanks for building this! Where should the initializeExtension() code go?
Hi, @eduardoacskimlinks thanks for building it to support MV3! I used this PR and found that I got store from Popup, but the state of Popup didn't sync with the state of the background script.
Let's say I made a React component and dispatched a thunk action to the background script to fetch API, but after the background script received the API response and updated the redux state, the state of Popup was not updated. I am sure the previous
webext-redux
works correctly. This issue only exists in this PR. Do you have any idea? Anyway, thanks for this nice patch again!
Hi @nasonlee28, thanks for the support. If you moved to Manifest V3 in your extension then webext-redux
won't work as the library relies on the chrome.runtime.connect
function. This is combined with the service-worker architecture where the background script is stopped to save resources. I am confident the original version doesn't work on V3.
Regarding your issue, it seems similar to the one we faced in the content-script. However, I need more details on the implementation to guide you through any potential solutions. For example, is your extension open source?
@z0ccc The setup is the same as before regarding webext-redux
. The only change is the package and the fact that you will require to persist the state as described in this PR
@eduardoacskimlinks Hi, thanks for the reply. I'm confused as to where clearState, saveState, etc come from. Are these functions I need to write myself?
@z0ccc That's correct. Initially, I added them as placeholders to make the code readable as you need to use chrome storage API. For instance, we use promises
+ chrome.storage.local
that you can find here, but I encourage you to find the best solution for you and your extension needs.
Hi, @eduardoacskimlinks thanks for building it to support MV3! I used this PR and found that I got store from Popup, but the state of Popup didn't sync with the state of the background script. Let's say I made a React component and dispatched a thunk action to the background script to fetch API, but after the background script received the API response and updated the redux state, the state of Popup was not updated. I am sure the previous
webext-redux
works correctly. This issue only exists in this PR. Do you have any idea? Anyway, thanks for this nice patch again!Hi @nasonlee28, thanks for the support. If you moved to Manifest V3 in your extension then
webext-redux
won't work as the library relies on thechrome.runtime.connect
function. This is combined with the service-worker architecture where the background script is stopped to save resources. I am confident the original version doesn't work on V3.Regarding your issue, it seems similar to the one we faced in the content-script. However, I need more details on the implementation to guide you through any potential solutions. For example, is your extension open source?
@z0ccc The setup is the same as before regarding
webext-redux
. The only change is the package and the fact that you will require to persist the state as described in this PR
Hi @eduardoacskimlinks, thanks for replying to me.
I have a reducer and action file:
import merge from 'lodash/merge';
export const openPopup = () => {
return (dispatch) => {
setTimeout(() => {
dispatch({ type: 'OPEN_POPUP' });
}, 5000);
};
};
export const uiOpenPopup = () => {
return {
type: 'UI_OPEN_POPUP',
};
};
const initialState = {
isPopupOpened: false,
};
export default (state = initialState, action) => {
const { type } = action;
switch (type) {
case 'OPEN_POPUP':
return merge({}, state, { isPopupOpened: true });
default:
return state;
}
};
And I applied the alias
middleware while creating store.
applyMiddleware(alias({ UI_OPEN_POPUP: openPopup }))
Here is how my popup dispatch action:
// In my react component
const Popup = () => {
useEffect(() => {
store.dispatch(uiOpenPopup());
}, []);
const isPopupOpened = useSelector((state) => state.test.isPopupOpened);
return <>{`isPopupOpened: ${isPopupOpened}`}</>;
};
const store = await getProxyStore();
ReactDOM.render(
<Provider store={store}>
<Popup />
</Provider>,
document.getElementById('app')
);
How I get store in Popup script:
import { Store } from '@eduardoac-skimlinks/webext-redux';
let store = null;
export const getProxyStore = async () => {
if (!store) {
store = new Store();
}
await store.ready();
return store;
};
The first time I opened the popup, I saw isPopupOpened: false
, after 5 secs it would dispatch openPopup
action to change the redux state, but my popup won't sync with the latest state. The popup still displays isPopupOpened: false
.
But If I opened the popup again, it will display isPopupOpened: true
which is expected.
Not sure if it is relative to MV3 or redux, I found that my console panel of popup shows the redux logs by redux-logger
which I use to log on the background script.
@nasonlee28 Sorry for the late reply, we were closing the quarter, and I didn't have much time available.
Regarding the issue, it seems precisely the one we faced on the content scripts on our side that drove us to create this PR. Have you used the persistence storage that I describe in the PR description? It's likely that the service worker goes to sleep and sends a new message with the original state causing the pop-up to reset to the initial store state otherwise.
I recommend searching on your bundle disabling minifying for the function serializedMessagePoster
responsible for notifying the tabs with the new state and both patchState
from the server and content store wrapper to gather more insights.
Finally, I don't discard that it may be an error on the PR, so we need to do some debugging to understand where the problem comes from Setup or the code itself.
Hi, guys! First of all, @eduardoacskimlinks thank you for your excellent work.
@nasonlee28, I’ve met with the same problem. Popup didn’t update its state after the background state had been updated. It happens because the background store doesn’t broadcast changes into the popup(arrow 4).
As a possible workaround, you can subscribe on chrome.storage change directly(arrow 5). Here is a code snippet:
proxyStore.ready().then(() => {
const update = (changes) => {
const newState = changes[STORAGE_CACHE_VERSION].newValue;
proxyStore.replaceState(newState);
};
chrome.storage.onChanged.addListener(update);
render(
<Provider store={proxyStore}>
<Popup />
</Provider>,
window.document.querySelector('#app-container')
);
});
Nonetheless, I’ve prepared a fix for it. We could notify Popup together with content-scripts inside serializedMessagePoster
. Here is Pull Request. Please, review when you have free time.
Also, I’ve made an example of working extension that uses fixes from my PR.
To test it:
https://user-images.githubusercontent.com/7114858/179461149-0493a13c-168c-4926-be25-4855c1b79475.mov
@RomanSavarin Thanks for contributing to the PR and providing great context. I will review your proposed changes and try to come back to you in a couple of days.
@nasonlee28 I have released the changes from @RomanSavarin into the following version @eduardoac-skimlinks/webext-redux@3.0.1-release-candidate
.
Kudos to @RomanSavarin, for the great explanation and example. There were helpful to try the changes
@eduardoacskimlinks, @RomanSavarin, @nasonlee28 , @z0ccc & everyone else who helped to make this happen: THANK YOU! I just replaced the dependency with your package and everything works as expected so far. I will do some more testing, but that's saving me and others so much time.
@svrnm Glad to hear. Hopefully, we can keep this project alive even if it's through a fork
After extensive experimentation, I found Redux isn't the most effective solution for scaling our Chrome extension. Check out my article for insights. Stay tuned for the follow-up on building a robust React UI extension without Redux. (@eduardoacskimlinks is company account 😄)
following up on this as well: things looked like they worked for a while but things are still not stable/breaking from time to time (even when using @eduardoac-skimlinks/webext-redux
unfortunately), it looks like we also need to redo a lot of the basics of the chrome extension
@svrnm That also happened to us; what I found, in general, is the broadcast effectively works, but we found two significant situations when it becomes cranky:
I would like to hear about your challenges and learn more about other developers experiences
@eduardoacskimlinks we never really could isolate a root cause of the issue, all we get is complaints by users of the extension (https://github.com/svrnm/demomonkey) that storing configurations is broken randomly (which let us to a similar assumption that there is somehow something not OK with the service workers). For some time we assumed that bugs in chrome are responsible (see https://bugs.chromium.org/p/chromium/issues/detail?id=1316588 and https://bugs.chromium.org/p/chromium/issues/detail?id=1426461), but after we thought it was fixed, it now returned once again.
Final Note: I have to admit that when I created that extension years ago I had some fundamental misunderstandings of React&Redux and a lot of stuff that shouldn't go through that mechanism, is flowing through it. It worked for multiple years just fine, so I never touched it 🤷♂️
cc @clintonfray
@eduardoacskimlinks, The issues we experiencing has the following symptoms:
Thanks for sharing for additional coverage. This is helpful because it validates that we are experiencing the same issues.
Hey @eduardoacskimlinks, thanks for working on this! I've cherry-picked these changes into #297 to make some additional fixes. Closing this PR in favor of #297.
297
That's fantastic news, @SidneyNemzer! I appreciate you taking the time to incorporate my work and make the additional fixes. I'll keep an eye on #297. My personal GitHub handler moving forward is @EduardoAC, so feel free to tag me if there's anything else I can do to help.
Supporting manifest v3 within webext-redux
The goal is to move away from connecting architecture into an event-driven model where the state changes will be broadcast across the tabs, allowing the backend-script store to sync within the different content-script.
How has the architecture changed?
We have adjusted the architecture by introducing an initialisation and broadcast mechanism which will follow three parts:
Resources
Fix #244