Skip to content

Conversation

@softworkz
Copy link
Contributor

The spread operator must not be used for the data parameter because the params are provided by data as an array and not as function arguments (which the spread operator would collect).
The way it was caused the data parameter to be wrapped in an array once again (n-size data array wrapped inside a single-element array), so the later spread operator unpacks the outer array instead of spreading out the data array as multiple parameters.

Copilot AI review requested due to automatic review settings November 11, 2025 22:53
Copilot finished reviewing on behalf of softworkz November 11, 2025 22:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a parameter handling bug in IPC renderer communication where the spread operator was incorrectly used in function parameter definitions, causing data to be double-wrapped in arrays.

  • Removed spread operator (...data) from sendToIpcRenderer and sendToIpcRendererBrowserView function parameter definitions
  • The data parameter is now correctly treated as an array that gets spread when passed to webContents.send()
  • Updated compiled JavaScript and source map files to reflect the TypeScript changes

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
src/ElectronNET.Host/api/ipc.ts Fixed parameter signatures in sendToIpcRenderer and sendToIpcRendererBrowserView handlers by removing spread operator from parameter definitions
src/ElectronNET.Host/api/ipc.js Compiled JavaScript output reflecting the TypeScript changes
src/ElectronNET.Host/api/ipc.js.map Updated source map file to match the corrected TypeScript and JavaScript code

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@softworkz
Copy link
Contributor Author

Hooray! The first PR in a long time which gets all green 😆

@agracio
Copy link

agracio commented Nov 11, 2025

I was just typing up a proposed solution based on electron-sharp but yours is a lot more straightforward and makes more sense now that we are not dependant on serializers.

For reference mine was

public void Send(BrowserWindow browserWindow, string channel, params object[] data)
{
    var objects = new List<object>
    {
        browserWindow,
        channel
    };
    
    foreach (var parameterObject in data)
    {
        objects.Add(parameterObject);
    }
    BridgeConnector.Socket.Emit("sendToIpcRenderer", objects.ToArray());
}

But since we are no longer working with serializers there really is no need for ...data

@softworkz
Copy link
Contributor Author

That's a valid solution when you can't change the other side.

But it can be simplified:

With C# 12.0 this could have been done most elegantly:

        public void Send(BrowserWindow browserWindow, string channel, params object[] data)
        {
            BridgeConnector.Socket.Emit("sendToIpcRenderer", [ browserWindow, channel, ..data]);
        }

With current C# version (10.0 for .net 6):

        public void Send(BrowserWindow browserWindow, string channel, params object[] data)
        {
            BridgeConnector.Socket.Emit("sendToIpcRenderer", new object[] { browserWindow, channel }.Concat(data).ToArray());
        }

@agracio
Copy link

agracio commented Nov 11, 2025

That is very elegant! I am offended that Resharper did not tell me about it usually it is very proactive with these types of optimisations.

@softworkz
Copy link
Contributor Author

Yea that's right, but this one is a bit too special.

@agracio
Copy link

agracio commented Nov 11, 2025

Why are we on .net 6, it is now out of support and compiler gives constant reminders on every build.

@softworkz
Copy link
Contributor Author

Why are we on .net 6, it is now out of support and compiler gives constant reminders on every build.

The nuget packages include both, net6 and net8 libs and we can add net10 as well once it's out.
I had kept net6 because the main tree had recently been updated to net8 and some people complained about it.
Supporting net6 doesn't really hurt us. It would also be possible to change the C# compiler for net6 to 12. These are only defaults/recommendations, but actually, the compiler is largely independent from the .net version - in the upward direction at least.

@agracio
Copy link

agracio commented Nov 11, 2025

I published a nuget package a year ago that is targeted at .NET Framework and dropped 4.5 leaving only 4.62. Even then people managed to complain without considering that 4.5 was dropped many years ago, I ignored them.

I think as long as .net 6 compiler does not warn of vulnerabilities we are good and switching to C# 12 is a great idea.

@softworkz
Copy link
Contributor Author

.net 6, it is now out of support

The kind of "support" they are talking about is rather limited from my experience. I've never seen any bug that I've been concerned about, being fixed in the same .net major version.
It must be a really huge-impact issue for this to happen. What they are doing is primarily security fixes and the other work that is done is more about tooling updates (the "hundreds ranges" in the SDK versions).

@softworkz
Copy link
Contributor Author

I think as long as .net 6 compiler does not warn of vulnerabilities we are good and switching to C# 12 is a great idea.

Wait - thinking about it: When I said it is fine to use a newer compiler, with older .net versions, then that's my experience from consuming projects. For the case of creating nuget packages for public consumption, I'd need to check out first whether my statement would still hold true.

@agracio
Copy link

agracio commented Nov 11, 2025

Not sure I understand your concerns.

Usually when nuget package targets multiple frameworks it would have multiple subfolders for each version. But it does depend on a package and it appears that nuget packager can determine whether multiple versions are needed at all and create everything accordingly.

Copy link
Collaborator

@FlorianRappl FlorianRappl left a comment

Choose a reason for hiding this comment

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

LGTM! Much appreciated

@FlorianRappl FlorianRappl merged commit 68feffb into ElectronNET:develop Nov 12, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants