refactor: use separate ipc-renderer-internal / ipc-main-internal APIs for Electron internals #13940
Conversation
lib/browser/api/ipc-main.js
Outdated
| } | ||
|
|
||
| // Do not throw exception when channel name is "error". | ||
| emitter.on('error', () => {}) |
There was a problem hiding this comment.
anybody knows why do we need this piece of code?
There was a problem hiding this comment.
@miniak Node.JS throws an unhandled exception if an event emitter emits error with no handlers. Having this handler stops that occuring. We definitely still need it 👍
There was a problem hiding this comment.
@MarshallOfSound restored
There was a problem hiding this comment.
There was a problem hiding this comment.
@MarshallOfSound ipcRenderer does not handle the error event compared to ipcMain
c8b07e9 t
8000
o
c357434
Compare
|
Labeling as breaking change as some users may be relying on Electron internals (sending / receiving Electron IPC events) |
|
@MarshallOfSound do you agree with the idea of separating the IPC channels for internal / public use? |
|
@miniak Yeah 100%, future iterations of this could completely hide the internal channel from developers but this is a great move 👍 |
spec/api-ipc-renderer-spec.js
Outdated
| describe('ipcRenderer.on', () => { | ||
| it('is not used for internals', (done) => { | ||
| w = new BrowserWindow({ show: false }) | ||
| w.webContents.once('did-finish-load', async () => { |
There was a problem hiding this comment.
w.loadURL('about:blank')
await emittedOnce(w.webContents, 'did-finish-load')
<...>
spec/api-ipc-main-spec.js
Outdated
| let appProcess = cp.spawn(electronPath, [appPath]) | ||
|
|
||
| appProcess.stdout.on('data', (data) => { output += data }) | ||
| appProcess.stdout.on('end', () => { |
There was a problem hiding this comment.
Probably it's easier to
appProcess.stdout.on('data', (data) => { output += data })
await emittedOnce(appProcess.stdout, 'end')
<...>| throw new TypeError('First argument has to be webContentsId') | ||
| } | ||
|
|
||
| ipcRenderer.send('ELECTRON_BROWSER_SEND_TO', true, false, webContentsId, channel, ...args) |
There was a problem hiding this comment.
Can you please add inline comments for what all these true, false mean?
doStuff(/* shouldWriteMoreCode */ true, /* willAddMoreComments */ false, ...)There was a problem hiding this comment.
@alexeykuzmin I've used constants to name the arguments instead, is that ok?
lib/browser/api/web-contents.js
Outdated
| } | ||
|
|
||
| WebContents.prototype._sendInternal = function (channel, ...args) { | ||
| if (channel == null) throw new Error('Missing required channel argument') |
There was a problem hiding this comment.
Maybe we should check the first arg type instead of comparing it with null and undefined?
if (typeof channel != 'string')|
@MarshallOfSound Why do you think it's a breaking change? |
|
@alexeykuzmin Some people will be using electron internal IPC to do things, for instance we have an app that sends an internal IPC message delibrately |
|
re: whether or not this deserves the That label is used as a reminder to not backport, so IMO this is really a discussion of whether or not to include in This diff is huge and we're late in the beta cycle and this isn't a blocker issue. It's safer to leave this targeted for |
|
@ckerr I agree that big changes shouldn't be merged into release branches if they cannot be thoroughly tested before a release should go stable. UPD We probably just need a new label "do-not-backport" to prevent backports. |
Some people shouldn't be doing that. What's the point of having public API and private implementation, if people keep using whatever they want? |
Hmm actually I like the I agree it doesn't cover this case though, so maybe a new label is needed in addition to the existing |
Quite possibly.
100% agree, that's why I'm advocating this goes into a major release, not that it goes through the "deprecation" cycle. I'm just pointing out that this can and will have side effects. The key example being people who use the events, but also people's tests. If someone is checking the amount of IPC listeners this will implicitly be broken now because the number will be slightly different. I'm on board with this going out, but only in a major. |
43e7e1e to
28503df
Compare
|
@MarshallOfSound Can you please review this again? |
b36dccd to
e3e28ce
Compare
|
@miniak This is coated in conflicts, if you get those cleaned up I'm down to land this 👍 |
4a9e37b to
e82796c
Compare
56da12d to
e5a3a03
Compare
|
@miniak I've tried to write my enforcement tests but it literally makes the electron test suite take ~10-15 minutes extra (which is pretty silly). I'm down to land this as-is |
923484f to
c00acaf
Compare
|
@MarshallOfSound rebased and conflicts resolved on the latest master. |
|
@miniak linux tests fail with a crash. |
|
@alexeykuzmin the crash seems to be completely unrelated to this change. |
|
c00acaf to
fdd6fa1
Compare
|
cc @nitsakh ^^ |
4997929 to
cf5347e
Compare
… for Electron internals
cf5347e to
77eb01e
Compare
|
Release Notes Persisted
|
remotemodule) are no longer using the publicipcRenderer/ipcMainmodulesipcRenderer.removeAllListeners()is now safe to callipcMain.removeAllListeners()is now safe to callEventEmitterallows access to all listeners)Checklist
npm testpassesNotes: Public IPC channel (
ipcRenderer/ipcMain) is no longer used to implement Electron internals (remotemodule for example)