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

Chrome Extension Background Page and useEffect Not Triggered #16629

Closed
mikecann opened this issue Aug 31, 2019 · 13 comments
Closed

Chrome Extension Background Page and useEffect Not Triggered #16629

mikecann opened this issue Aug 31, 2019 · 13 comments

Comments

@mikecann
Copy link

mikecann commented Aug 31, 2019

Do you want to request a feature or report a bug?

bug

What is the current behavior?

When using useEffect from a background page in a chrome extension the body of the effect isnt triggered.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

See the demo project here: https://github.com/mikecann/bgpage-hooks-issue

What is the expected behavior?

useEffect should be triggered

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

It stopped working somewhere between react 16.8 and 16.9 , you can change the react and react_dom version in the demo project and see that it works as expected in 16.8

Workaround

For now the issue can be worked around as demonstrated: https://github.com/mikecann/bgpage-hooks-issue/blob/master/bg.tsx#L5

So im not really sure whats going on but perhaps because background pages arent actually visually rendered to the screen then React has optimized things so that effects arent triggered?

@mikecann
Copy link
Author

Perhaps related to: #16606 ??

@Jack-Works
Copy link
Contributor

Perhaps related to: #16606 ??

That should be not related. #16606 only happened in Firefox content script, but this issue happened in Chome background page (and why run React in the background page?)

@mikecann
Copy link
Author

mikecann commented Sep 1, 2019

Perhaps related to: #16606 ??

That should be not related. #16606 only happened in Firefox content script, but this issue happened in Chome background page (and why run React in the background page?)

Well im wondering if its something to do with requestAnimationFrame? Are react useEffect's now triggered by requestAnimationFrame? Perhaps requestAnimationFrame doesnt work in background pages?

and why run React in the background page?

Because I find the useState and state update cycle super useful for creating reactive logic in the background. I can create custom hooks that can be used in both the background and other extension pages.

@Jack-Works
Copy link
Contributor

Yes, if user cannot see the page, rAF will not be called

@Jack-Works
Copy link
Contributor

You can replace rAF with setTimeout 0 to make it work in background page.

@Jack-Works
Copy link
Contributor

And I have made a hooks without react demo few days ago

interface Hooks {
    useState<T>(initialValue: T): [T, (newVal: T) => void]
    useEffect(f: () => () => void): void
}
function useHooks(fn: (hooks: Hooks) => void) {
    const called: ({ type: 'state'; data: [any, (val: any) => void] } | { type: 'effect'; data: () => void })[] = []
    let currentIndex = 0

    let pendingUpdate = false
    function callFn() {
        if (pendingUpdate === true) return console.debug('An update is pending. Skip this re-render.')
        pendingUpdate = true
        // undo useEffect
        called.forEach(x => x.type === 'effect' && x.data())
        currentIndex = 0
        fn(hooks)
        pendingUpdate = false
    }

    const hooks: Hooks = {
        useState<T>(init: T) {
            if (called[currentIndex] === undefined) {
                const index = called.length
                called.push({
                    type: 'state',
                    data: [
                        init,
                        val => {
                            const that = called[index]
                            if (that.type !== 'state') throw new TypeError('You must call hooks in the same order')
                            that.data[0] = val
                            callFn()
                        }
                    ] as [T, (val: T) => void]
                })
            }
            const result = called[currentIndex]
            if (result.type !== 'state') throw new TypeError('You must call hooks in the same order')
            currentIndex++
            return result.data
        },
        useEffect(f) {
            if (called[currentIndex] === undefined) {
                called.push({ type: 'effect', data: f() })
            } else {
                const that = called[currentIndex]
                if (that.type !== 'effect') throw new TypeError('You must call hooks in the same order')
                if (that.data) that.data()
                that.data = f()
            }
            currentIndex++
        }
    }
    return callFn
}
const myFunc = useHooks(({ useState, useEffect }) => {
    const [state, setState] = useState(0)
    console.log('Current state', state)
    useEffect(() => {
        const f = () => setState(state + 1)
        document.addEventListener('click', f)
        return () => document.removeEventListener('click', f)
    })
})
myFunc()

@mikecann
Copy link
Author

mikecann commented Sep 1, 2019

@Jack-Works very nice indeed! One problem however is the rule that hooks number and order must be preserved, so one way I usually get around that is by creating components that have hooks in them.

An example might be notifications. From the background page you can create "Notification" components which call the chrome.notifications api when added, removed, updated etc.

Obviously the above is possible without components but sometimes its nice to model them in that way even if the DOM isnt visible.

Yes, if user cannot see the page, rAF will not be

So this is confirmation that from 16.9 onwards React is broken in chrome extension background pages?

@Jack-Works
Copy link
Contributor

They are using raf now. But I don't know if they use it in the past.

And I think it's a edge case to use react in the background page. I don't know if they will fix it.
You can replace window.raf with setTimeout before react loads to make it work currnetly

@mikecann
Copy link
Author

mikecann commented Sep 1, 2019

And I think it's a edge case to use react in the background page. I don't know if they will fix it.
You can replace window.raf with setTimeout before react loads to make it work currnetly

Do you have a quick example of how this would look?

@Jack-Works
Copy link
Contributor

{
    "name": "Background hooks test",
    "version": "0.0.1",
    "manifest_version": 2,
    "permissions": ["notifications"],
    "background": {
        "scripts": ["polyfill.js", "react.development.js", "react-dom.development.js", "demo.js"]
    }
}
window.requestAnimationFrame = f => setTimeout(f, 0)
window.cancelAnimationFrame = i => clearTimeout(i)
"use strict";
function ChromeNotification(props) {
    const { notificationID, onClick, message, title } = props;
    console.log('ID:', notificationID);
    const [val, setVal] = React.useState(0);
    if (val < 100)
        setTimeout(() => {
            setVal(val + 1);
        }, 300);
    React.useEffect(() => {
        chrome.notifications.create(notificationID, {
            message,
            type: 'progress',
            iconUrl: icon,
            title,
            progress: val
        });
        return () => chrome.notifications.clear(notificationID);
    }, [notificationID]);
    React.useEffect(() => {
        chrome.notifications.onClicked.addListener(onClick);
        return () => chrome.notifications.onClicked.removeListener(onClick);
    });
    React.useEffect(() => {
        chrome.notifications.update(notificationID, { message, title, progress: val }, x => console.log(x ? 'Notification updated' : 'Notification not updated'));
    }, [message, title, val]);
    return null;
}
function NotificationFromEvent() {
    const instanceID = React.useRef(Math.random().toString());
    const [title, setTitle] = React.useState('Title');
    const [message, setMessage] = React.useState('Message');
    React.useEffect(() => {
        const f = (e) => setMessage(e.detail);
        document.addEventListener('a', f);
        return () => document.removeEventListener('a', f);
    });
    React.useEffect(() => {
        const f = (e) => setTitle(e.detail);
        document.addEventListener('b', f);
        return () => document.removeEventListener('b', f);
    });
    return (React.createElement(ChromeNotification, { title: title, message: message, notificationID: instanceID.current, onClick: () => {
            console.log('Notification clicked!');
        } }));
}
const notification = React.createElement(NotificationFromEvent, null);
ReactDOM.render(notification, document.body.appendChild(document.createElement('div')));
Object.assign(globalThis, {
    a: (x) => {
        document.dispatchEvent(new CustomEvent('a', { detail: x }));
    },
    b: (x) => {
        document.dispatchEvent(new CustomEvent('b', { detail: x }));
    }
});
var icon = `REPLACE WITH YOUR ICON`;

@mikecann
Copy link
Author

mikecann commented Sep 3, 2019

@Jack-Works Sweet! Thanks :)

@gaearon
Copy link
Collaborator

gaearon commented Mar 16, 2020

Is this fixed in 16.13.0? Can you check?

@mikecann
Copy link
Author

@gaearon Yep this now works as expected in 16.13.0 :) Thanks Dan!

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

No branches or pull requests

4 participants