Fixed types to prevent naming collision and to allow direct import from BufferList

[alexandercerutti]

Following what has been discussed inside #114 with @piranna and @rvagg, I tried to apply some changes so that BufferList.d.ts could be used just like:

// CommonJS
const { BufferList } = require("bl/BufferList.js");

const BufferList = require("bl/BufferList.js");

// CommonJS + TS
import BufferList = require("bl/BufferList.js");

To do so I had to wrap BufferList.d.ts in a typescript namespace and add the discussed export = to the end of the file. I also renamed the exports because they don't seem to have an actual meaning but I think they had a sort of naming collision.

The most significant challenge, after having added export = to BufferList.d.ts, was to allow exporting BufferListAcceptedTypes and BufferListConstructor so that index.d.ts could import them.

I've performed several tests with several imports in VSCode and they seemed to work fine. Let me know if everything works as expected.

These are the imports I tested in test/test.js:

const BufferListStream = require("..");

new BufferListStream();
BufferListStream();

BufferListStream.BufferList();
new BufferListStream.BufferList();

const { BufferListStream } = require("..");

new BufferListStream();
BufferListStream();

BufferListStream.BufferList();
new BufferListStream.BufferList();

const BufferList = require("../BufferList.js");

new BufferList();
BufferList();

Let me know!

[piranna]

Can you test it using import instead of require()?

[piranna]

I confirm that import BufferList from 'bl/BufferList' doesn't works :-(

[alexandercerutti]

@piranna I don't think it is supposed to work. I mean, there is no property default in the code that actually says that BufferList should be importable like that.
I think the correct ways are:

import { BufferList } from "bl/BufferList";
import BufferList = require("bl/BufferList");

[piranna]

Maybe we should add it? What I see It wrong is to need to use a named export when importing BufferList, being in fact the only export of that file... Nothing wrong about the named export, just only that default one is missing and we must add it, because It was working with 5.x.x version and Definitely Typed types.

OTOH, import BufferList = require("bl/BufferList"); is wrong, it's a mixture of ESM and CommonJS that only works because BabelJS is transpiling and unifying them.

[alexandercerutti]

I honestly don't see how this could have worked with types on DT 🤔 Maybe I'm missing something, but I don't see the default export in there.

I tried to setup a small sample test with DT types and [email protected]. This is the result.

Open for tsconfig.json

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "esModuleInterop": false,
    "forceConsistentCasingInFileNames": true
  }
}

With my types, the ones already published, the error message is different.

In both cases, turning on compilerOptions.esModuleInterop make them work correctly, so I think you are maybe getting fooled by that option. Is it possible?

Anyway, applying the changes in this PR on the project, I'm getting back to the DT types error.

So I guess this is fine.

OTOH, import BufferList = require("bl/BufferList"); is wrong, it's a mixture of ESM and CommonJS that only works because BabelJS is transpiling and unifying them.

It is a Typescript signature for importing CommonJS things.
https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require

[piranna]

I've tried myself removing the BL namespace and replacing exports with

declare const BufferList: BufferListConstructor;

export = BufferList;

and it works on node_modules, but tests fails. It also give me the esModuleInterop, and in fact I have it enabled in my code, but using it to import types in another module in the same project itself, I feel it hacky...

Offtopic, but IMHO i would remove CommonJS support and allow just only ESM, that would simplify things.

[alexandercerutti]

That's curious. I don't have any issues, as I said, with both tests and importing in a new project. Having the namespace is the only valid solution I found that allows both CommonJS and exporting the types.

but using it to import types in another module in the same project itself, I feel it hacky...

I kind of agree, but that flag allows us to make CommonJS work with ESM. I mean, that's correct when we are running in an ESM environment and importing a CommonJS package.

But that's not the case. Tests are written and run in CommonJS, so it is correct, for me, to not have esModuleInterop enabled.

Offtopic, but IMHO i would remove CommonJS support and allow just only ESM, that would simplify things.

I agree, but that's up to @rvagg. Why don't you open a new issue proposing this?

[mcollina]

It's totally possible to write types that works for ESM, commonjs, TypeScript etc. It's not simple but it's doable. Take a look at fastify/fastify#4349.

Regarding migrating anything of the JS side to ESM.. that's an hard no for me.

[alexandercerutti]

@mcollina thank you! I'll take a look at the issue you linked, but the fact here is that bl does not have a default support, so I guess that wouldn't be compatible with ESM default importing. So it actually makes no sense to me to have an export default signature, or something like that, in Typescript if the code does not actually support it without esModuleInterop flag on.

Regarding migrating anything of the JS side to ESM.. that's an hard no for me.

May I ask you why? Still too breaking?

[mcollina]

Unfortunately I don't have enough time to look into this but it's totally possible, I think it just requires some careful definition on the JS side as well.

May I ask you why? Still too breaking?

pretty much. Libraries like bl should aim for maximum compatibility. (It's ok to use ESM for applications)

[piranna]

Is there anything I can do to move this forward?

[alexandercerutti]

Hey everyone, bl's typings on DT are now getting deprecated!.

How can we bring this on?

[piranna]

How can we bring this on?

Maybe copy the DefinitelyTyped types, and update them here? I think just only the 64 bits support are the missing APIs...

[alexandercerutti]

No, the issues with DTs were, also, that you weren't able to call BufferList and BufferListStream as a function, but only via new. A lot of things changed, and we worked so hard to reach this point. I wouldn't have started the deprecation phase on DT in that case (which, for instance, was a bit difficult and heavy to handle due to other dependencies).

So, to understand better, what is the problem you are facing with these type changes? I thought we solved.

[piranna]

So, to understand better, what is the problem you are facing with these type changes? I thought we solved.

As said at #117 (comment), by using import BufferList from 'bl/BufferList as an ESM import, it doesn't works.

[piranna]

I have just tried with import BufferList = require("bl/BufferList.js"); and it's giving me the next error:

error TS1202: Import assignment cannot be used when targeting ECMAScript 6 or higher. Consider using 'import * as ns from "mod"', 'import {a} from "mod"' or 'import d from "mod"' instead.

How can we move this forward? It seems support for TypeScript with ESM output is broken in v6.0.0...

[alexandercerutti]

I have just tried with import BufferList = require("bl/BufferList.js"); and it's giving me the next error:

I think this is okay actually. I mean, Typescript is telling you that if you are compiling for >= ES6 ("module"), you should not use CommonJS syntax. You should use the suggested syntax instead.

---

Anyway, sorry for the late reply. I was missing Node has a compatibility mode for ESM, so even if a module misses the default property, you still can use the syntax import x from "y".

As said at #117 (comment), by using import BufferList from 'bl/BufferList as an ESM import, it doesn't works.

Are you trying them (both DT and new) in a clean project? And, when you applied changes, did you try to restart TS Server? Because DT typings weren't working with the default import and esModuleInterop: false, as I showed you above, so I'm not sure we should add that.

That said, I might try to add the support, but I'm a bit in contrast. I mean:

• ✅ import BufferList from "bl/BufferList"; + esModuleInterop: true
• ✅ import { BufferList } from "bl/BufferList";
• ❌ import BufferList from "bl/BufferList"; + esModuleInterop: false

What confuses me the most is that the Typescript team tries to enforce the usage of esModuleInterop: true so that it can be fully compliant with ES specifications. So my question is: should we actually "fix" this and support the syntax without esModuleInterop? And, moreover, is that actually possible with what @mcollina suggested? 🤔

[piranna]

I think this is okay actually. I mean, Typescript is telling you that if you are compiling for >= ES6 ("module"), you should not use CommonJS syntax. You should use the suggested syntax instead.

The exact error is (in spanish):

No se puede usar una asignación de importación cuando se eligen módulos de ECMAScript como destino. Considere la posibilidad de usar "import * as ns from 'mod'", "import {a} from 'mod'", "import d from 'mod'" u otro formato de módulo en su lugar.ts(1202)

I have tried all the other suggested combinations, and none of them worked. In fact, it gives me an error about not finding the module (wtf?).

Are you trying them (both DT and new) in a clean project? And, when you applied changes, did you try to restart TS Server? Because DT typings weren't working with the default import and esModuleInterop: false, as I showed you above, so I'm not sure we should add that.

I have not tried in a clean project, but definitely I "restart the server" each time since I'm running tsc by hand :-)

import { BufferList } from "bl/BufferList";

By doing it that way, I get two errors, in VSCode I get "BufferList" solo se puede importar si se usa una importación predeterminada.ts(2595), and on terminal I get

src/Streamer/index.ts:319:10 - error TS2532: Object is possibly 'undefined'.

319    yield (initializationSegment).shallowSlice(begin, end)
             ~~~~~~~~~~~~~~~~~~~~~~~

src/Streamer/index.ts:319:34 - error TS2339: Property 'shallowSlice' does not exist on type 'BufferListConstructor'.

319    yield (initializationSegment).shallowSlice(begin, end)
                                     ~~~~~~~~~~~~

src/Streamer/index.ts:552:3 - error TS2741: Property 'isBufferList' is missing in type 'BufferList' but required in type 'BufferListConstructor'.

552   this.#initializationSegment = bufferList.shallowSlice(
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/bl/BufferList.d.ts:24:3
    24   isBufferList(other: unknown): boolean;
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'isBufferList' is declared here.

[piranna]

What's more annoying, is that I have esModuleInterop: true, so import BufferList from "bl/BufferList"; should work. Maybe it's not doing it because I'm transpiling into ESM modules instead of CommonJS?

On a side note, what about migrating the project to ESM, generate a CommonJS version on prepublish with Babel or something else, and publish the package with both versions on the exports field? Maybe it can help here and also prepare us for the future... What do you think @mcollina?

[piranna]

I've been able to fix the types at alexandercerutti#1, or at least it's working now for my use case :-)

[mcollina]

Take a look at fastify/fastify#4349. There a few caveats in supporting

• cjs
• esm
• ts
• ts nodenext

[alexandercerutti]

I'm sorry everyone, in these days I'm having a few personal troubles so I cannot dedicate much time to this :( I'll everything check asap (likely on the next week)

[piranna]

Take care @alexandercerutti :-)