8000 feat: add ContextBridgeMutability feature by nikitakot · Pull Request #27348 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add ContextBridgeMutability feature #27348

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 22, 2021
Merged

feat: add ContextBridgeMutability feature #27348

merged 2 commits into from
Mar 22, 2021

Conversation

nikitakot
Copy link
Contributor
@nikitakot nikitakot commented Jan 18, 2021 •
edited
Loading

Description of Change

This PR adds a new base::Feature - ContextBridgeMutability. When the feature is enabled values that are exposed via context bridge are not freezed and are not made readonly.

ContextBridgeMutability off
image

ContextBridgeMutability on
image

As discussed with @MarshallOfSound no need to document it.

Checklist

Release Notes

Notes: Adding ContextBridgeMutability feature that skips context bridge DeepFreeze and SetReadOnlyNonConfigurable when exposing a value.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jan 18, 2021
@nikitakot
Copy link
Contributor Author

p.s. - haven't wrote any tests for this, but happy to do it if someone explains me how to do something like app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts.

@MarshallOfSound
Copy link
Member

haven't wrote any tests for this, but happy to do it if someone explains me how to do something like app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts.

You should be able to do exactly that.

@nikitakot
Copy link
Contributor Author

haven't wrote any tests for this, but happy to do it if someone explains me how to do something like app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts.

You should be able to do exactly that.

Can you be more concrete please? Calling app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts seems like too late, the parameter doesn't setup. It works if I put it in spec-main/index.js though, but then that parameter is set for all other tests too.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jan 25, 2021
@zcbenz
Copy link
Contributor
zcbenz commented Feb 15, 2021

Can you be more concrete please? Calling app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts seems like too late, the parameter doesn't setup. It works if I put it in spec-main/index.js though, but then that parameter is set for all other tests too.

You can start a new Electron app and read its output, for example:
https://github.com/electron/electron/blob/master/spec-main/api-menu-spec.ts#L908-L922

@zcbenz zcbenz added the semver/minor backwards-compatible functionality label Feb 15, 2021
@nikitakot nikitakot force-pushed the context-bridge-mutability branch from e487796 to 7409330 Compare February 15, 2021 20:03
@nikitakot
Copy link
Contributor Author

Can you be more concrete please? Calling app.commandLine.appendSwitch('enable-features', 'ContextBridgeMutability'); in spec-main/api-context-bridge-spec.ts seems like too late, the parameter doesn't setup. It works if I put it in spec-main/index.js though, but then that parameter is set for all other tests too.

You can start a new Electron app and read its output, for example:
https://github.com/electron/electron/blob/master/spec-main/api-menu-spec.ts#L908-L922

done

@nikitakot nikitakot force-pushed the context-bridge-mutability branch 2 times, most recently from bbb5a47 to b105042 Compare February 15, 2021 21:50
@nikitakot nikitakot force-pushed the context-bridge-mutability branch from b105042 to dc55919 Compare February 15, 2021 22:19
@nornagon
Copy link
Contributor

What's the reasoning for this? It seems questionably useful to me.

@nikitakot
Copy link
Contributor Author

What's the reasoning for this? It seems questionably useful to me.

For the case when you're modifying exposed values in a existing codebase and don't wanna refactor it when you enable context isolation.

@nikitakot
Copy link
Contributor Author

What's the reasoning for this? It seems questionably useful to me.

@nornagon does the reason I posted makes sense to you? we've used this functionality in our electron fork, but I'm ok with abandoning this PR if you don't wanna merge it

@nornagon
Copy link
Contributor

Hm, I'm skeptical that this is broadly useful. It's possible to replicate this behavior with a Proxy, which I think is more flexible and clearer (it's surprising, i think, that any modifications aren't reflected back to the isolated world).

I'm OK to merge this behind the flag as it's quite a small change and well-tested, but I don't think this should ever be "sanctioned" behavior.

@nikitakot
Copy link
Contributor Author
nikitakot commented Mar 19, 2021 •
edited
Loading

it's surprising, i think, that any modifications aren't reflected back to the isolated world)

why? objects in isolated and main worlds are completely different objects

let's merge it then

@nornagon
Copy link
Contributor

@nikitakot that follows if you're familiar with the implementation details of contextBridge, but for a naive user I think it's surprising, because in other ways contextBridge seems "seamless", and feels like a regular JS object.

@nornagon nornagon merged commit e99893d into electron:master Mar 22, 2021
@release-clerk
Copy link
release-clerk bot commented Mar 22, 2021

Release Notes Persisted

Adding ContextBridgeMutability feature that skips context bridge DeepFreeze and SetReadOnlyNonConfigurable when exposing a value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniak miniak miniak approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
api-review/requested 🗳 semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nikitakot @MarshallOfSound @zcbenz @nornagon @miniak
0