fix: senderId removed in Electron 28

[pbi-qfs]

Since the property has been removed with Electron 28

[pbi-qfs]

Any ETA for this fix to land in the main branch?

[codebytere]

I don't think this fully addresses the issue; with 28.x we see:

remote on git:main ❯ yarn install                                                      5:00PM
yarn install v1.22.19
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
success Saved lockfile.
$ tsc
src/renderer/remote.ts:314:15 - error TS2551: Property 'senderId' does not exist on type 'IpcRendererEvent'. Did you mean 'sender'?

314     if (event.senderId !== 0) {
                  ~~~~~~~~

  node_modules/electron/electron.d.ts:6002:5
    6002     sender: IpcRenderer;
             ~~~~~~
    'sender' is declared here.

src/renderer/remote.ts:315:81 - error TS2551: Property 'senderId' does not exist on type 'IpcRendererEvent'. Did you mean 'sender'?

315       console.error(`Message ${channel} sent by unexpected WebContents (${event.senderId})`);
                                                                                    ~~~~~~~~

  node_modules/electron/electron.d.ts:6002:5
    6002     sender: IpcRenderer;
             ~~~~~~
    'sender' is declared here.

[pbi-qfs]

Yes, senderId has been removed, so it can only be checked for backward compatibility with older Electron versions.

If the build fails with Electron 28, can it be worked around by using event['senderId'] instead? Or should we simply remove the gatekeeper completely?

[dsanders11]

I'd be in favor of using event['senderId'] for forward compatibility, personally.

[dsanders11]

@pbi-qfs, could you add 28 to the CI config below this line so we can ensure whatever final form gets merged passes CI for Electron 28?

remote/.circleci/config.yml

Line 50 in 642040d

- 27

[Kilian]

It seems event['senderId'] is also caught by tsc:

$ tsc
src/renderer/remote.ts:314:28 - error TS2551: Property 'senderId' does not exist on type 'IpcRendererEvent'. Did you mean 'sender'?

314     const senderId = event['senderId']
                               ~~~~~~~~~~

test/all.ts:890:23 - error TS2345: Argument of type '"unhandledRejection"' is not assignable to parameter of type '"loaded"'.

890           process.off('unhandledRejection', onUnhandledRejection)
                          ~~~~~~~~~~~~~~~~~~~~


Found 2 errors.

I'm not a typescript expert but I wonder if if ('senderId' in event) { as a wrap around it would work.

[felixrieseberg]

Proposal: Make a custom interface IpcRendererEventWithSenderId extends IpcRendererEvent interface and make a type checker function that returns if it matches. That way you're not tricking TS and being explicit about what you're doing.

// Backwards compatibility interface for Electron < v28
interface IpcRendererEventWithSenderId extends Electron.IpcRendererEvent {
	senderId: number
}

function hasSenderId(input: any): input is IpcRendererEventWithSenderId {
	return typeof input.senderId === "number"
}

[nornagon]

Seems reasonable given sendTo has been removed in Electron 28, so the sender of messages will always be the main process, which is what this check was trying to enforce anyway.

[nornagon]

Looks like the tests are failing due to some other change in Electron 28.

[pbi-qfs]

Proposal: Make a custom interface IpcRendererEventWithSenderId extends IpcRendererEvent interface and make a type checker function that returns if it matches. That way you're not tricking TS and being explicit about what you're doing.

// Backwards compatibility interface for Electron < v28
interface IpcRendererEventWithSenderId extends Electron.IpcRendererEvent {
	senderId: number
}

function hasSenderId(input: any): input is IpcRendererEventWithSenderId {
	return typeof input.senderId === "number"
}

Sounds great - can someone please take over this pull request, who knows more of typescript than my "it's almost Javascript but with another file extension" approach...

[dsanders11]

Latest test failures are due to electron/electron#40501 and this line in this code base:

remote/src/main/server.ts

Line 388 in 642040d

customEvent.returnValue = process.mainModule!.require(moduleName)

[Kilian]

I implemented the proposal of @felixrieseberg and now tsc is happy, but as @dsanders11 noted we now have to the issue from #40501.

Do we need to implement a test/workaround for that in remote as well so we support 28.0.0 (with the bug), or is it safe to wait for #40501 to land in 28.0.1 or w/e and then merge this?

This bug is unfortunately a blocker for me, so I'd love to get it past the finish line.

[dsanders11]

@Kilian, I don't think a fix is coming for that issue, per Sam's comment: electron/electron#40501 (comment)

Sounds like process.mainModule is going to stay gone, so this likely needs a workaround.

[Kilian]

I replaced process.mainModule with a loop to get the topmost module (module.parent is also deprecated so it's not ideal, but it does work) and logging mainModule.filename in that function gives the expected result, but I can't figure out how to update the test for it, since the test seems to use the process.mainModule logic as well:

it('should search module from the user app', async () => {
      expectPathsEqual(
        path.normalize(await remotely(() => require('./renderer').process.mainModule!.filename)),
        path.resolve(__dirname, 'index.js')
      )
})

I tried updating it similarly by requiring renderer, and then looping over its parents to find the mainModule, but require('./renderer') has no .parent in the test. Any tips/suggestions?

Stale review, issue addressed

[continuous-auth]

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀