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

fix: senderId removed in Electron 28 #171

Merged
merged 9 commits into from
Dec 12, 2023
Merged

Conversation

pbi-qfs
Copy link
Contributor

@pbi-qfs pbi-qfs commented Nov 15, 2023

Since the property has been removed with Electron 28

src/renderer/remote.ts Outdated Show resolved Hide resolved
@pbi-qfs pbi-qfs changed the title Accepting ipc events without senderId fix: Accepting ipc events without senderId Nov 15, 2023
@pbi-qfs
Copy link
Contributor Author

pbi-qfs commented Nov 27, 2023

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

@@ -311,7 +311,7 @@ function metaToError (meta: { type: 'error', value: any, members: ObjectMember[]

function handleMessage (channel: string, handler: Function) {
ipcRenderer.on(channel, (event, passedContextId, id, ...args) => {
if (event.senderId !== 0) {
if (event.senderId !== 0 && event.senderId !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

@dsanders11
Copy link
Member

@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?

Since the property has been removed with Electron 28
@Kilian
Copy link
Member

Kilian commented Dec 6, 2023

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.

…8, where "senderId" is removed from IpcRendererEvent
@felixrieseberg
Copy link
Member

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"
}

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

nornagon commented Dec 6, 2023

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

@pbi-qfs
Copy link
Contributor Author

pbi-qfs commented Dec 6, 2023

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
Copy link
Member

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

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

@Kilian
Copy link
Member

Kilian commented Dec 8, 2023

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
Copy link
Member

@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 Kilian changed the title fix: Accepting ipc events without senderId Electron 28 support Dec 11, 2023
@Kilian
Copy link
Member

Kilian commented Dec 11, 2023

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?

@dsanders11 dsanders11 changed the title Electron 28 support fix: senderId removed in Electron 28 Dec 12, 2023
@dsanders11 dsanders11 dismissed codebytere’s stale review December 12, 2023 19:44

Stale review, issue addressed

@VerteDinde VerteDinde merged commit 51ff1b4 into electron:main Dec 12, 2023
3 checks passed
Copy link

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants