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

Use execute script to reload page rather than chrome.tabs.reload() #17

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

gbro3n
Copy link
Contributor

@gbro3n gbro3n commented Dec 8, 2020

This is a better fix to the previous PR.

Calling chrome.tabs.reload (tabs[0].id) is unreliable as the call to chrome.runtime.reload() kills the operation before it can complete. Executing location.reload() with a timeout in the tab page will complete however.

… chrome.runtime.reload() causes the previous operation to abort.
@xpl xpl merged commit 5f7683c into xpl:master Dec 9, 2020
xpl pushed a commit that referenced this pull request Dec 9, 2020
xpl pushed a commit that referenced this pull request Dec 9, 2020
@xpl
Copy link
Owner

xpl commented Dec 9, 2020

@garethrbrown Hello, the fix is nice, but I have found that executeScript requires adding extra tab/origin permissions to manifest.json which might be unwanted and also breaks the backward compatibility.

At first I came up with that code which checks if a permission has been granted:

const reload = () => {
    chrome.permissions.contains ({
        origins: ["http://*/*", "https://*/*"]
    }, granted => {
        if (granted) {
            chrome.tabs.query ({ active: true, lastFocusedWindow: true }, tabs => { // NB: see https://github.com/xpl/crx-hotreload/issues/5
                if (tabs[0]) {
                    chrome.tabs.executeScript (tabs[0].id, { code: 'setTimeout(() => { location.reload() }, 300)' }, () => {})
                    chrome.runtime.reload ()
                }
            })
        } else {
            alert ('Unable to reload the active tab — please add "http://*/*" and "https://*/*" permissions to manifest.json!')
            chrome.runtime.reload ()
        }
    })
}

...but then I thought that maybe we could simply reload the tab upon extension's startup. So the reload() function goes away and the code becomes that simple:

const watchChanges = (dir, lastTimestamp) => {
    timestampForFilesInDirectory (dir).then (timestamp => {
        if (!lastTimestamp || (lastTimestamp === timestamp)) {
            setTimeout (() => watchChanges (dir, timestamp), 1000) // retry after 1s
        } else {
            chrome.runtime.reload () // <---- reload the runtime only
        }
    })
}

chrome.management.getSelf (self => {
    if (self.installType === 'development') {
        chrome.runtime.getPackageDirectoryEntry (dir => watchChanges (dir))
        chrome.tabs.query ({ active: true, lastFocusedWindow: true }, tabs => { // <--- reload the tab upon startup
            if (tabs[0]) {
                chrome.tabs.reload (tabs[0].id)
            }
        })
    }
})

This does have a side effect — when you launch the browser for the first time, it would reload the active tab, because the extension couldn't know if it's been reloaded or not. But as long as it happens only on a developer's machine, that is a minor price for maintaining the code simple and not requiring extra permissions.

Let me know your thoughts on that.

P.S. Also note that I have changed currentWindow: true to lastFocusedWindow: true as it was suggested long time ago in #5. I found it a better option because it allows reloading the tab while being focused on the extension's DevTools open in a separate window. Let me know if that doesn't work well.

@gbro3n
Copy link
Contributor Author

gbro3n commented Dec 9, 2020

Have tested out your fix and can confirm works for me. The original issue of injected content remains solved and if your fix requires fewer manifest permissions then that's a win. Thanks for the good work :)

@xpl
Copy link
Owner

xpl commented Dec 9, 2020

The fix has gone into master. Thanks for you work too!

@gbro3n gbro3n deleted the reload-set-timeout branch December 9, 2020 14:20
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.

2 participants