Skip to content

Consider providing override for platform detection in JS/TS SignalR client #49065

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

Open
mitchdenny opened this issue Jun 28, 2023 · 3 comments
Open
Labels
area-signalr Includes: SignalR clients and servers

Comments

@mitchdenny
Copy link
Member

mitchdenny commented Jun 28, 2023

An internal team reached out with a problem they are experiencing with our platform detection logic:

https://github.com/dotnet/aspnetcore/blob/7918436a914ac1cbafa94a359e435c67535c8159/src/SignalR/clients/ts/signalr/src/Utils.ts#L40C9-L40C9

Locally this works, but they are seeing a problem in production where it is detecting that it is in the browser instead of Node. This could possibly be due to global namespace pollution from another package that is impacting their code that runs locally.

They asked whether it would be possible to provide the ability to override the platform detection logic.

Another possibility is that we could see whether there is a better way to detect that we are running in Node vs. looking for the presence of the window global variable.

@mitchdenny
Copy link
Member Author

After looking at the way that Platform.isNode (and friends) are used. I think plumbing through some kind of override from the top level APIs would be pretty ugly with lots of code churn in the library. Given the problem here is global namespace polution - perhaps a little bit more global namespace solution could be our salvation?

What if we allowed someone to set global variables:

signalRPlatformDetectionOverride = "node";

Then Platform.isNode and friends would check this as part of its logic.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@josep-llodra
Copy link

Today I needed something like this:

signalRPlatformDetectionOverride = "browser";

Why? I am using Electron and I need it to have nodeIntegration: true and contextIsolation: false.

Hence, Platform.isNode returns true, and require('ws') throws.

So, I manually modified the Platform class to return what I needed and works perfectly.

@jjjaniszewski
Copy link

I had a same issue as above. I'm trying to use SignalR inside renderer of Electron application. For me it throws with require('eventsource'). Initially i tried to import it from preload script but it only causes chain reaction of require problems.

For now i have solved it with hacky monkey patching:

import {Platform} from '@microsoft/signalr/dist/esm/Utils';

// Override the methods
Object.defineProperty(Platform, 'isNode', {
  get: () => false,
  configurable: true,
});

Object.defineProperty(Platform, 'isBrowser', {
  get: () => true,
  configurable: true,
});

I'm not quite sure what should be proper solution but probably something like:

static get isNode() {
  return window == undefined && typeof process !== "undefined" && process.release && process.release.name === "node";
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

No branches or pull requests

4 participants