@aspnet/signalr has a hard dependency on node packages even though it is designed for usage in browser.

[MicahZoltu]

Describe the bug

@aspnet/signalr TypeScript definition files reference node types.

To Reproduce

Steps to reproduce the behavior:

1. npm init
2. npm install @aspnet/signalr
3. Add tsconfig.json

   {
       "compilerOptions": {
           "target": "es2015",
           "module": "es2015",
           "moduleResolution": "node",
           "outDir": "output",
           "rootDir": "source",
           "strict": true,
           "noEmitOnError": true,
           "lib": [ "dom", "es2015" ]
       },
       "include": [
           "source/**/*.ts"
       ]
   }

4. Create a file source/index.ts import * as signalR from '@aspnet/signalr' signalR
5. npx tsc
6. Notice the two errors:

   node_modules/@aspnet/signalr/dist/esm/IHubProtocol.d.ts:1:23 - error TS2688: Cannot find type definition file for 'node'.
   1 /// <reference types="node" />
                           ~~~~
   node_modules/@aspnet/signalr/dist/esm/IHubProtocol.d.ts:130:49 - error TS2580: Cannot find name 'Buffer'. Do you need to install type definitions for node? Try `npm i @types/node`.
   130     parseMessages(input: string | ArrayBuffer | Buffer, logger: ILogger): HubMessage[];
                                                       ~~~~~~
   Found 4 errors.

Expected behavior

Expected @aspnet/signalr to be usable in a browser without having to polyfill node.

[MicahZoltu]

This actually appears to be a more deep seeded problem than just this. It appears that the browser bundle doesn't actually match the definition files in the project. So while import { HubConnectionBuilder } from '@aspnet/signalr' type checks, at runtime it will fail if you are using dist/browser/signalr.js because that bundle doesn't export a HubConnectionBuilder.

It would seem that there isn't typescript definitions for the browser bundle at all. What is weird is that this project includes ESM build, but the ESM build doesn't work in browser (see https://github.com/Zoltu/typescript-transformer-append-js-extension/#motivation) but it also won't work in NodeJS since NodeJS ES modules require an .mjs suffix. So I'm not sure who the ES module is targeting exactly... maybe bundlers?

---

Side note: If you use the transformer linked above during your TSC build (assuming this project is TypeScript, I haven't actually checked) then you will be able to use it directly in a browser without bundling, which would be cool.

[BrennanConroy]

Expected @aspnet/signalr to be usable in a browser without having to polyfill node.

To be clear, there is no node polyfill necessary. In your case the suggestion was to install the @types/node package as a dev dependency so you can get type checking while developing. Having a @types package is a fairly common thing to do with npm packages.

Is your scenario above a common one? And do you have any suggestions on how to make it work? We've generally been suggesting using WebPack and have a tutorial based on that https://docs.microsoft.com/aspnet/core/tutorials/signalr-typescript-webpack?view=aspnetcore-2.2&tabs=visual-studio

[MicahZoltu]

You are correct that installing @types is a common solution, but it is uncommon to require the user to install @types for a package they won't be using. The reason installing @types/node is problematic is because it means if I put something like process.exit() into my code it will compile fine but I'll end up with a runtime failure because window.process doesn't exit at runtime.

As to how to resolve the surface problem of the dependency on the Type for Buffer the easy way is to just define a type within the package that looks enough like a Buffer to satisfy your needs. Since TS is duck typed, this will match the Node buffer if the project is used in Node, and for browser projects it will still compile just fine. You can even call the type Buffer within SignalR.

As for the broader problem of there being no types for the browser side, that is a bit more involved. My personal suggestion is to compile the ES6 for browser, which means (at the moment) using a transformer like I linked in my comment above.

Another option is to compile down to a single file, but make it a single ES6 file. This would make it so you don't need the transformer, but it would work natively in ES6 and the types would line up.

For a project like this with an established userbase, the best solution is probably to add a new output that would sit along side the current UMD output and ES6 and CommonJS output that would target ES6 browser (single file or J's extension added).

[MicahZoltu]

Is your scenario above a common one?

To answer this directly, I think it is relatively uncommon right now, but becoming more common now that all browsers have ES6 module support. I think once named imports lands in browsers (needs polyfill at the moment) my use case will continue to grow. Essentially, what I'm doing is "bleeding edge" browser work (utilizing all native features and not using legacy tooling) and while it isn't the most common use case, it is the most likely case for the ecosystem to move into.

[analogrelay]

So I'm not sure who the ES module is targeting exactly... maybe bundlers?

Correct, the ESM build is currently targetting bundlers.

This is definitely a scenario we need to revisit. Our primary focus for the ESM module has been WebPack and similar bundlers, but now that ESM support is starting to come online, it will be good to look at it. We'll have to evaluate the customer demand for it against other work, but I think it's a reasonable request.

[MicahZoltu]

Perhaps this should be a separate issue (let me know if so and I can create one) but the dist/browser/signalr.js doesn't have a matching dist/browser/signalr.d.ts. This means that currently if you are building a web app and use that, you cannot write TypeScript code at all.

I think the fix for this is much simpler. However you are generating that file, you just need to make sure to generate a matching .d.ts. I glanced at this repo and it wasn't immediately obvious how that file is being generated, but assuming it is just using TypeScript compile-to-single-file feature, then it is just a matter of enabling the declaration: true compilerOption in tsconfig.json. If you are using something like webpack or rollup to turn the ES6 module compile into a single file, I believe both tools have a mechanism for also reducing the type definitions to a single file (with the appropriate transformations to UMD module).

[analogrelay]

the dist/browser/signalr.js doesn't have a matching dist/browser/signalr.d.ts

The .d.ts files in the ESM folder are supposed to work here. We use TypeScript's export as namespace signalR feature to allow those typings to serve dual purpose. That's why they're referenced from the package.json in the typings property. If imported as a module, they work as a module; if imported as a script, then all the members of the module are added to a global-scope variable called signalR (which is how the browser UMD version works). Adding @microsoft/signalr to the compilerOptions.types array in your tsconfig.json should bring in those definitions.

We don't ship an @types/ package for two main reasons: 1) We include type definitions in our core package already and 2) there already exists an @types/signalr package but it's for the ASP.NET SignalR client which has a different version scheme and API. We could re-examine that decision as well. It's not unreasonable (I believe) to have an @types/microsoft-signalr package that simply re-exports @microsoft/signalr, but we haven't explored that in depth.

assuming it is just using TypeScript compile-to-single-file feature, then it is just a matter of enabling the declaration: true compilerOption in tsconfig.json ... If you are using something like webpack or rollup to turn the ES6 module compile into a single file, I believe both tools have a mechanism for also reducing the type definitions to a single file (with the appropriate transformations to UMD module).

Unfortunately, since we need to bundle a few dependencies, we aren't just using tsc to do the bundling. We use webpack and I investigated .d.ts combiner plugins but didn't find anything that was resilient enough (most just do simple concatenation with some very basic heuristics). Happy to re-examine this if you've got thoughts/ideas here.

In general, for 5.0 I think we want to re-examine our JS client. This will be the third time we re-examine how we build it, but that's not entirely surprising given how the JS world changes :). Any feedback, suggestions, contributions you have would be very welcome here!

Perhaps this should be a separate issue

I think it's fine to discuss bundling/distribution altogether in this thread.

[MicahZoltu]

if imported as a script, then all the members of the module are added to a global-scope variable called signalR (which is how the browser UMD version works).

Ah, this is what I was missing. I did some additional testing and was able to get SignalR working in a browser without bundling using ES6, but I had to make 2 edits to the distributed definition files:

1. Delete line 1 of IHubProtocol.d.ts:

   /// <reference types="node" />

2. Delete the reference to Buffer down on 127 and 130:

   parseMessages(input: string | ArrayBuffer | Buffer, logger: ILogger): HubMessage[];

---

Solutions time

Switch from Buffer to ArrayBuffer in that function, and drop the dependency on Buffer, and thus node, from the project entirely.

It appears that there are two IHubProtocol implementations in TypeScript, JsonHubProtocol.ts and MessagePackHubProtocol.ts

JsonHubProtocol doesn't actually support the input parameter as an ArrayBuffer (side-node: I'm not sure how that code compiles, since parseMessage doesn't properly implement IHubProtocol.parseMessage).

MessagePackHubProtocol does two things with input. First it does some input validation on it and then it passes it to BinaryMessageFormat.parse which only accepts an ArrayBuffer! So already we can assert that MessagePackHubProtocol.parseMessage can only accept ArrayBuffer, since it passes the input into a function that only accepts ArrayBuffer.

However, we can keep digging and see that BinaryMessageFormat.parse's input parameter is only used as a constructor argument to Uint8Array and the input.byteLength property is read. Since Uint8Array constructor takes Iterable<number> | ArrayLike<number> | ArrayBuffer | SharedArrayBuffer, we can thus constrain the type to:

(Iterable<number> | ArrayLike<number> | ArrayBuffer | SharedArrayBuffer) & {byteLength:number}

And this would all be without making a breaking change, since Buffer satisfies those constraints any code that was passing in a Buffer before would continue to work.

Proposal

I propose you drop Buffer from signalr since it appears to be the only nodejs specific code in the whole project and replace the one place that depends on it with ArrayBuffer. This is because the change is simple and Buffer is a ArrayBuffer for all intents and purposes. I would also change the type checking such that instead of testing for instanceof Buffer, you instead do a ducktype test to see if the thing has the properties you care about, such as by looking for a .byteLength property on the object.

Alternatively, you could change the type requirement to the intersection of the Uint8Array constructor arguments and {byteLength:number}. This would narrow the type requirements even more and allow a broader selection of potential parameters to be passed into that function.

---

With this relatively minor change, I believe you could fully remove the dependency on Buffer and thus on node types, which would allow this project to work out-of-the-box in modern browsers.

[MicahZoltu]

I did a bit more digging and there are maybe half a dozen places internally where you are using Buffer. I believe these could all be trivially replaced with Uint8Array, and I encourage doing so so that you can fully remove the dependency on Buffer internally in the project as well, but that is out of scope of this particular request. For the sake of just getting rid of the external type dependency on Buffer I believe the proposal above will do the trick.

[analogrelay]

Originally, the issue with dropping Buffer was that the MessagePack protocol library (@microsoft/signalr-protocol-msgpack) uses a MessagePack library that requires Buffer (and provides a polyfill for browsers). So that's one thing that gets in the way a bit. I do think we can resolve this though. We have been thinking about switching to a MessagePack library based solely on native JS typed arrays.

[Hawxy]

I'm unsure if its related, but the @microsoft/signalr package breaks various types within my Typescript Vue.js project (TS 3.6.3 + Vue-CLI v4 RC3) when imported like import * as signalR from "@microsoft/signalr" or import { HubConnectionBuilder } from "@microsoft/signalr". The errors don't appear while editing, but during compile I get stuff like:

36:18 Parameter 'r' implicitly has an 'any' type.
    34 |         await http.url(`...`)
    35 |             .delete()
  > 36 |             .res(r => {
       |                  ^
    37 |                 ...
    38 |             });
    39 |     }

(the response type for the Wretch package gets broken globally)

If I change the import to const signalR = require("@microsoft/signalr"); as suggested on the npm page everything works fine (but I lose type safety). This might catch some people out that refer to the Webpack + TS example and wonder why their TS SPA isn't compiling.

[Brendan-csel]

I've also having issues as a side-effect of this. Because signalr is causing the node types to be imported, I'm getting typescript build errors due to differing return values of setTimeout (NodeJS.Timeout versus number from lib.dom.d.ts for the browser).

I can "fix" by removing the lines mentioned above from IHubProtocol.d.ts. Of course I can also work around this by not being fussy about the return type of setTimeout ...but still the ambient node types are not needed for anything else in this project and can't be great to have them hanging around.

For clarity the "fix" was Delete line 1 of IHubProtocol.d.ts:

/// <reference types="node" />

Delete the reference to Buffer down on 135:

parseMessages(input: string | ArrayBuffer | Buffer, logger: ILogger): HubMessage[];

[stasberkov]

While this is not fixed you can use the following https://www.npmjs.com/package/patch-package package to patch @microsoft/signalr package during npm install. Here is how to:

1. Install @microsoft/signalr.
2. Patch node_modules\@microsoft\signalr\dist\esm\IHubProtocol.d.ts file by removing /// <reference types="node" /> line and Buffer type usage from line parseMessages(input: string | ArrayBuffer | Buffer, logger: ILogger): HubMessage[];.
3. Run npx patch-package @microsoft/signalr command. It will create patches folder with instructions how to patch original packages after npm install procedure.
4. Add file from patches folder to your git repository.
5. Add "postinstall": "patch-package" into scripts section in package.json.
6. Run npm i patch-package --save. This will add patch-package to you npm dependencies.

[stasberkov]

Why Microsoft is not fixing this trivial (as it looks to me) issue for more than year already? Does lack of attention to such issues mean that signalr will be abandoned soon and it is preferable to switch to gRPC?