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

refactor: resolve browser via env variables based on build rather than user agent #27179

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Aug 2, 2023

Fixes #26911, #26860.

Currently, we are parsing user agent string to determine which browser is running the extension. This doesn't work well with custom user agents, and sometimes when user turns on mobile dev mode in Firefox, we stop resolving that this is a Firefox browser, extension starts to use Chrome API's and fails to inject.

Changes:
Since we are building different extensions for all supported browsers (Chrome, Firefox, Edge), we predefine env variables for browser resolution, which are populated in a build step.

@hoxyq hoxyq force-pushed the devtools/resolve-browser-based-on-build branch from 74a1390 to e2aec5a Compare August 2, 2023 14:14
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Aug 2, 2023
@hoxyq hoxyq changed the title refactor: resolve browser via env variables based on build, not by user agent refactor: resolve browser via env variables based on build rather than user agent Aug 2, 2023
@ansh-ag
Copy link

ansh-ag commented Aug 6, 2023

Im not sure if this is the correct place to post this but curious to discuss,
shouldn't our extensions be the same for all chromium based browser ? why should we have separate extension for edge or chrome in that case ? Wouldn't that cover a bigger pool of browsers ?

@hoxyq
Copy link
Contributor Author

hoxyq commented Aug 7, 2023

Im not sure if this is the correct place to post this but curious to discuss, shouldn't our extensions be the same for all chromium based browser ? why should we have separate extension for edge or chrome in that case ? Wouldn't that cover a bigger pool of browsers ?

Yes, essentially Chrome and Edge extensions are almost the same. There are some small differences, like in manifest files, where we need to provide text description of the extension, which can be customised for different browsers, or setting icons logic in runtime.

@hoxyq hoxyq merged commit 2c4c847 into facebook:main Aug 29, 2023
2 checks passed
@hoxyq hoxyq deleted the devtools/resolve-browser-based-on-build branch August 29, 2023 09:40
hoxyq added a commit that referenced this pull request Aug 29, 2023
List of changes:
* refactor: refactored devtools browser extension scripts to improve
port management and service worker lifetime
([hoxyq](https://github.com/hoxyq) in
[#27215](#27215))
* refactor[devtools/extension]: minify production builds to strip
comments ([hoxyq](https://github.com/hoxyq) in
[#27304](#27304))
* fix[devtools]: allow element updates polling only if bridge is alive
([hoxyq](https://github.com/hoxyq) in
[#27067](#27067))
* refactor: resolve browser via env variables based on build rather than
user agent ([hoxyq](https://github.com/hoxyq) in
[#27179](#27179))
* fix[devtools/updateFiberRecursively]: mount suspense fallback set in
timed out case ([hoxyq](https://github.com/hoxyq) in
[#27147](#27147))
* Feat:-Added open in editor to appear by default
([Biki-das](https://github.com/Biki-das) in
[#26949](#26949))
* fix[devtools/inspect]: null check memoized props before trying to call
hasOwnProperty ([hoxyq](https://github.com/hoxyq) in
[#27057](#27057))
* rename SuspenseList export to unstable_SuspenseList
([noahlemen](https://github.com/noahlemen) in
[#27061](#27061))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…n user agent (facebook#27179)

Fixes facebook#26911,
facebook#26860.

Currently, we are parsing user agent string to determine which browser
is running the extension. This doesn't work well with custom user
agents, and sometimes when user turns on mobile dev mode in Firefox, we
stop resolving that this is a Firefox browser, extension starts to use
Chrome API's and fails to inject.

Changes:
Since we are building different extensions for all supported browsers
(Chrome, Firefox, Edge), we predefine env variables for browser
resolution, which are populated in a build step.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
List of changes:
* refactor: refactored devtools browser extension scripts to improve
port management and service worker lifetime
([hoxyq](https://github.com/hoxyq) in
[facebook#27215](facebook#27215))
* refactor[devtools/extension]: minify production builds to strip
comments ([hoxyq](https://github.com/hoxyq) in
[facebook#27304](facebook#27304))
* fix[devtools]: allow element updates polling only if bridge is alive
([hoxyq](https://github.com/hoxyq) in
[facebook#27067](facebook#27067))
* refactor: resolve browser via env variables based on build rather than
user agent ([hoxyq](https://github.com/hoxyq) in
[facebook#27179](facebook#27179))
* fix[devtools/updateFiberRecursively]: mount suspense fallback set in
timed out case ([hoxyq](https://github.com/hoxyq) in
[facebook#27147](facebook#27147))
* Feat:-Added open in editor to appear by default
([Biki-das](https://github.com/Biki-das) in
[facebook#26949](facebook#26949))
* fix[devtools/inspect]: null check memoized props before trying to call
hasOwnProperty ([hoxyq](https://github.com/hoxyq) in
[facebook#27057](facebook#27057))
* rename SuspenseList export to unstable_SuspenseList
([noahlemen](https://github.com/noahlemen) in
[facebook#27061](facebook#27061))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…n user agent (#27179)

Fixes #26911,
#26860.

Currently, we are parsing user agent string to determine which browser
is running the extension. This doesn't work well with custom user
agents, and sometimes when user turns on mobile dev mode in Firefox, we
stop resolving that this is a Firefox browser, extension starts to use
Chrome API's and fails to inject.

Changes:
Since we are building different extensions for all supported browsers
(Chrome, Firefox, Edge), we predefine env variables for browser
resolution, which are populated in a build step.

DiffTrain build for commit 2c4c847.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
3 participants