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

fix: prevent errors when extensions listen for chrome.tabs events #39729

Merged
merged 2 commits into from Sep 7, 2023

Conversation

codebytere
Copy link
Member

Description of Change

Fixes two errors in Chrome extensions that are thrown when extensions listen for chrome.tabs. Initially, these weren't added to our tabs.json file as we didn't initially intend to support them. However, the way that extension generation works, omitting them from the JSON file actually means that attempting to listen to them at all will throw an error, which we don't want. We don't need to implement them or make them work as intended in order to define them as valid events to listen to here. No documentation or tests are changed as support has not changed as a result of this PR.

Checklist

Release Notes

Notes: Fixed an error where listening to certain chrome.tabs events would throw incorrectly.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. labels Sep 4, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 4, 2023
@deepak1556
Copy link
Member

However, the way that extension generation works, omitting them from the JSON file actually means that attempting to listen to them at all will throw an error, which we don't want

Shouldn't the extensions avoid exercising these API when targeting Electron ? Maybe I am missing context, is the plan here for extensions to be able to run in Electron without any modifications, if that is the case what would be the error signal to extension authors when they use any undefined API ?

@codebytere
Copy link
Member Author

codebytere commented Sep 4, 2023

@deepak1556 this cropped up initially because several of the main devtools extensions for web frameworks make use of these events (ex. onCreated and onUpdated), including React DevTools. I'm not sure I think it's reasonable for them by default to check whether the event is defined to prevent the entire extension from failing to run before using it given that the extensions are widely used, important for debugging, and Electron is the only context where they'd run into an issue.

They're not documented as being supported - would it perhaps help if we explicitly noted we did not support them? Prior to this PR we defined onZoomed even though we didn't actually support it for similar reasons (though I plan to potentially add support for this now that #39428 has landed).

@deepak1556
Copy link
Member

Thanks for the context, do these extensions work fine when these tab events are declared but not implemented in Electron ? I see most of them as lifecycle related events, not sure if they cause any undefined behavior.

For me an error signal seems like the most explicit option for extensions, since Electron is not providing a 1:1 mapping of API from Chrome. However, would also love to hear thoughts from other maintainers on what they think is the best path.

@codebytere
Copy link
Member Author

codebytere commented Sep 5, 2023

@deepak1556

do these extensions work fine when these tab events are declared but not implemented in Electron ? I see most of them as lifecycle related events, not sure if they cause any undefined behavior.

yes - before adding this i did a brief audit of the most commonly used devtools extensions and found that's how they were indeed used.

The thing about extension events generally that makes them tricky is we can't do what we've done for some unsupported functions and e.g. log a warning to console indicating it won't work as expected while allowing the extension to continue to function otherwise. That's what would imo be ideal here but the way extension generated functionality works forces our hand a bit here, and unfortunately these events not being implemented is a direct blocker to #36545

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 5, 2023
@codebytere codebytere force-pushed the on-added-created-events branch 2 times, most recently from 769e417 to 8073f5d Compare September 6, 2023 08:06
@deepak1556
Copy link
Member

There is a "deprecated" property supported by the JSON compiler, can we use it to log error messages for the unsupported API and events ? Sample https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/extensions/api/tabs.json

@codebytere
Copy link
Member Author

codebytere commented Sep 6, 2023

@deepak1556 oh great idea - i'll do that for now! done!

@codebytere codebytere merged commit 75bad34 into main Sep 7, 2023
17 checks passed
@codebytere codebytere deleted the on-added-created-events branch September 7, 2023 10:41
@release-clerk
Copy link

release-clerk bot commented Sep 7, 2023

Release Notes Persisted

Fixed an error where listening to certain chrome.tabs events would throw incorrectly.

@trop
Copy link
Contributor

trop bot commented Sep 7, 2023

I have automatically backported this PR to "25-x-y", please check out #39770

@trop trop bot removed the target/25-x-y PR should also be added to the "25-x-y" branch. label Sep 7, 2023
@trop
Copy link
Contributor

trop bot commented Sep 7, 2023

I have automatically backported this PR to "27-x-y", please check out #39771

@trop
Copy link
Contributor

trop bot commented Sep 7, 2023

I have automatically backported this PR to "26-x-y", please check out #39772

@trop trop bot added in-flight/27-x-y in-flight/26-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed target/27-x-y PR should also be added to the "27-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. in-flight/27-x-y in-flight/25-x-y in-flight/26-x-y labels Sep 7, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
…lectron#39729)

* fix: prevent errors when extensions listen for chrome.tabs events

* chore: mark events unsupported in Electron
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants