Navigation Menu

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

feat: add WebContents.ipc #34959

Merged
merged 9 commits into from Aug 3, 2022
Merged

feat: add WebContents.ipc #34959

merged 9 commits into from Aug 3, 2022

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Jul 18, 2022

Description of Change

This adds a new API, WebContents.ipc, which works like ipcMain but is
scoped to only IPC messages sent from a single WebContents.

This allows a developer to more ergonomically communicate with a specific
WebContents, replacing either "source checks" or "channel checks" with a
"declarative" source/channel check.

e.g. without:

const w = new BrowserWindow
w.loadURL('app://window')
w.webContents.on('ipc-message', (event, channel, ...args) => {
  if (channel === 'test') {
    // ...
  }
})

// Or, for `invoke`:
ipcMain.handle('some-message', (event, ...args) => {
  if (event.sender === w.webContents) {
    // ...
  }
})

refactored to use the new WebContents.ipc API introduced in this PR:

const w = new BrowserWindow
w.loadURL('app://window')
w.webContents.ipc.on('test', (event, ...args) => {
  // ...
})

w.webContents.ipc.handle('some-message', (event, ...args) => {
  // event.sender is guaranteed to be === w.webContents
  // ...
})

I believe this is clearer and less error-prone than the existing methods for
accomplishing the same result.

Also adds an analogous API to WebFrameMain.ipc.

Checklist

Release Notes

Notes: Added new WebContents.ipc and WebFrameMain.ipc APIs.

@nornagon nornagon added no-backport semver/minor backwards-compatible functionality labels Jul 18, 2022
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Jul 18, 2022
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the API's current form, I find it a bit confusing for users without historical perspective that the win.webContents.ipc object would be separate from the existing win.webContents.send API.

My initial thought was to alias the IPC command to win.webContents.ipc.send, but then the object description gains a new dimension of complexity (e.g. can't be reduced to a one-line "an IpcMain instance").

@nornagon
Copy link
Member Author

Yeah, it seems reasonable to expect webContents.ipc.send to exist. We could clone the existing IpcMain docs over to IpcWebContents or something.

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

We should deprecated ipc-message and ipc-message-sync

@nornagon
Copy link
Member Author

@miniak I don't see any particular reason to deprecate them, they're minimal overhead and deprecating/removing them would break apps for no particular reason.

I'd be fine adding a note in the docs about the new ipc object though.

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

btw I tried doing this before in #16420 and it got rejected :)

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

@miniak I don't see any particular reason to deprecate them, they're minimal overhead and deprecating/removing them would break apps for no particular reason.

I'd be fine adding a note in the docs about the new ipc object though.

ok, makes sense

@nornagon
Copy link
Member Author

@miniak yeah, i remember that! thanks for the link. i now think i was wrong 😅

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

any plans to backport, at least to 20-x-y?

@nornagon
Copy link
Member Author

I don't think this is worth backporting. It's easily polyfillable.

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

In the API's current form, I find it a bit confusing for users without historical perspective that the win.webContents.ipc object would be separate from the existing win.webContents.send API.

My initial thought was to alias the IPC command to win.webContents.ipc.send, but then the object description gains a new dimension of complexity (e.g. can't be reduced to a one-line "an IpcMain instance").

win.webContents.send is just a shortcut for win.webContents.mainFrame.send :)

@miniak
Copy link
Contributor

miniak commented Jul 20, 2022

shouldn't the ipc property on webContents be also read-only? it is on the webFrameMain

@nornagon
Copy link
Member Author

nornagon commented Jul 20, 2022

I feel a little awkward about adding the ipc.send family of methods, because it adds to the already somewhat bewildering set of IPC methods available.

  • webContents.send
  • webContents.sendToFrame
  • webContents.postMessage
  • webFrameMain.send
  • webFrameMain.postMessage

and this would add:

  • webContents.ipc.send
  • webContents.ipc.postMessage
  • webContents.ipc.sendToFrame (maybe we want to elide this one as it's covered by webFrameMain.send now?)
  • webFrameMain.ipc.send
  • webFrameMain.ipc.postMessage

all of which would be duplicating functionality.

Does this seem worth it @erickzhao? I do agree that webContents.ipc.send makes sense given webContents.ipc.on('message') exists. But I'm not sure if it's so much better that it outweighs the awkwardness of a bunch of duplicated methods. WDYT?

@miniak
Copy link
Contributor

miniak commented Jul 21, 2022

@nornagon I don't think it's worth adding those methods. Moreover ipcMain also does not have send / postMessage

@miniak
Copy link
Contributor

miniak commented Jul 21, 2022

It's easily polyfillable.

are you sure? basic on, etc. could be, but not handle IMO

Co-authored-by: Samuel Attard <sam@electronjs.org>
@nornagon
Copy link
Member Author

@miniak yeah i guess it's not super trivial to do handle. I started writing out a polyfill, and it involved patching WebContents.prototype._init, so probably a bit overcomplicated :)

that said, Electron 20 will be going stable in about 2 weeks, and I don't feel super great about that short of a runway for this feature.

given this is just ergonomic and doesn't add any new functionality, I'm erring on the side of not backporting.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 25, 2022
@miniak
Copy link
Contributor

miniak commented Jul 27, 2022

API LGTM

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

i'm in agreement that #34959 (comment) is a case where drawbacks outweigh benefits, and that we shouldn't backport here.

@nornagon nornagon merged commit 6d859dc into main Aug 3, 2022
@nornagon nornagon deleted the webcontents-ipc branch August 3, 2022 23:55
@release-clerk
Copy link

release-clerk bot commented Aug 3, 2022

Release Notes Persisted

Added new WebContents.ipc and WebFrameMain.ipc APIs.

@miniak
Copy link
Contributor

miniak commented Aug 4, 2022

/trop run backport-to 21-x-y

@trop
Copy link
Contributor

trop bot commented Aug 4, 2022

The backport process for this PR has been manually initiated - sending your PR to 21-x-y!

@trop trop bot mentioned this pull request Aug 4, 2022
@trop
Copy link
Contributor

trop bot commented Aug 4, 2022

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

@trop trop bot added the in-flight/21-x-y label Aug 4, 2022
trop bot pushed a commit that referenced this pull request Aug 4, 2022
@miniak miniak removed the no-backport label Aug 4, 2022
@trop trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Aug 17, 2022
rzhao271 pushed a commit to rzhao271/electron that referenced this pull request Oct 10, 2022
feat: add WebContents.ipc (electron#34959)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/requested 🗳 merged/21-x-y PR was merged to the "21-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants