v6.0.0 - BigInt support & readable-stream@4

[rvagg]

Closes #109 & #87, incorporates #87 and #113, /cc @yuyaryshev & @piranna

[rvagg]

will bump semver-major because of the Node.js 12 drop in support (dropped already in CI but this forces it in functionality) and also readable-stream@4, which also drops Node.js 12—I believe this is the only real breaking change from there.

[mcollina]

lgtm

[piranna]

I'm getting the next typescript errors with this PR:

src/Streamer/index.ts:348:20 - error TS2351: This expression is not constructable.
  Type 'typeof import("/home/piranna/Trabajo/Dyte/LL-HLS-streamer_bttf/node_modules/bl/BufferList")' has no construct signatures.

348  #bufferList = new BufferList
                       ~~~~~~~~~~

src/Streamer/index.ts:427:17 - error TS2709: Cannot use namespace 'BufferList' as a type.

427  #onBox = (box: BufferList) => {
                    ~~~~~~~~~~

src/Streamer/mp4Parser/box.ts:1:1 - error TS6133: 'BufferList' is declared but its value is never read.

1 import BufferList from 'bl/BufferList'
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This didn't happened with @types/bl package, it seems the typescript definitions are wrong.

[piranna]

I've compared with the @types/bl implementation, and seems the main difference is usage of class instead of interface. Maybe this can be the reason? At least for some of the issues...

[piranna]

Ok, found the issue: it got fixed by adding export = BufferList; at the end of file BufferList.d.ts :-)

[piranna]

Ok, found the issue: it got fixed by adding export = BufferList; at the end of file BufferList.d.ts :-)

I tried to do a PR with this change but something odd is happening when running the tests. I'm not a Typescript expert, so maybe I've missed something, but definitely when setting that line in my node_modules, it works.

[rvagg]

@alexandercerutti would you mind weighing in here? See @piranna's error output above.

Since it's already out in a minor release and this isn't going to change anything I'm not going to treat TS problems as block this, we can follow-up with a fix.

[piranna]

I'm not going to treat TS problems as block this, we can follow-up with a fix.

Fair enough.

[github-actions]

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[alexandercerutti]

I've chosen to use interface due to how ones can interact with bl's API.
At the current situation, bl can be created via new, via function call or have static methods.

The fact that we have that function call forced me to use interface. @types/bl do not allow this.

I don't know if having bl/BufferList as import source is a planned feature. I mean, main is bl.js and it (along with index.d.ts) contains all that matters for creating a new Bufferlist.

[rvagg]

I think that if/when we go to ESM and have an exports map, "BufferList.js" (or perhaps just "BufferList") is probably going to be on the list along with the default. That export would have the nice property of not pulling in readable-stream where you care about your dependency tree being compact. So we should probably export it from BufferList.d.ts so it's resolved properly when you're just consuming BufferList.js?

[rvagg]

^ my point about ESM is just instructive, today require('bl/BufferList.js') has the same effect

[piranna]

We can add an exports field in the package.json file right now, there's no need to wait to use ESM... they are not inter-dependent features.

[rvagg]

@piranna yeah but it's not really necessary and may be a breaking change because I think it excludes people using imports with different names? (require('bl/BufferList.js') vs require('bl/BufferList'), even require('bl/bl.js')). I'm just trying to reinforce your point that we probably need to export from BufferList.d.ts for those that are importing just that module.

[piranna]

@rvagg just to be sure, I'm talking about https://nodejs.org/api/packages.html#package-entry-points. Are we talking about the same thing? If you are concerned about importing using extensions, we can add an entry for them too.

Regarding adding default export to BufferList.d.ts, yes, we need it :-)

[rvagg]

@piranna yes, and I'm talking specifically about this point in that document:

Existing packages introducing the "exports" field will prevent consumers of the package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json'). This will likely be a breaking change.

Right now, without an "exports" field, you can reach in and require/import arbitrary files in this package, such as BufferList.js.

Sorry, this is a bit of a distraction, I'm just talking about the fact that people want to consume just BufferList.js and we should make sure to support that.

[piranna]

I'm just talking about the fact that people want to consume just BufferList.js and we should make sure to support that.

Yes! And I'm one of them! :-) What I say is, we can be able to add ALL the files that we want to export, so people (and me! :-D) can still be able to access :-) Something like this:

{
  "exports": {
    ".": "./index.js",
    "./index": "./index.js",
    "./index.js": "./index.js",
    "./BufferList": "./BufferList.js",
    "./BufferList.js": "./BufferList.js",
  }
}

A bit bloated to add both extension and extensionless entries, but you get the idea, and the same for the types.

If this is not what you are talking about please clarify, because I don't get your point... :-/

[rvagg]

uhh, yeah we've diverged a long way from the original point, so let's drop this, you're correct but it's not really relevant; we're dealing with this from @alexandercerutti:

I don't know if having bl/BufferList as import source is a planned feature. I mean, main is bl.js and it (along with index.d.ts) contains all that matters for creating a new Bufferlist.

the answer being - yes, it's a planned feature, it's in wide usage, so we should allow for it and I think that means we need an export from BufferList.d.ts but I'm also not exactly an expert in handling typedefs.

[alexandercerutti]

@rvagg I guess the way to achieve this is as @piranna said and as I set on the bottom of index.d.ts:

bl/index.d.ts

Line 88 in a59710d

export = BufferListStream;

So, when importing bl, export will expose what comes from bl.js.
When importing bl/BufferList(.js), export will expose what has been declared.

More infos about export = available here.

[piranna]

I guess the way to achieve this is as @piranna said

Yes, I copied the idea from index.d.ts file, and it works on node_modules, but for some reason npm test fails when applying the changes in the code.

[alexandercerutti]

I'll check it later @piranna. @rvagg what about moving this discussion to a discussion topic or another issue? I'm losing notifications as this is already been merged :D

[piranna]

what about moving this discussion to a discussion topic or another issue? I'm losing notifications as this is already been merged :D

Never used discussions, but I think it's ok for me. Regarding notifications, I still get them in my mail...

[piranna]

And thank you for taking a look on this :-)

[alexandercerutti]

@piranna @rvagg I've tried to add that line, but it seems to conflict with other exports (ES6-style exports). I'm actually a bit stuck there because index.d.ts uses Bufferlist.d.ts things and I don't know how to export both types and things with export =. I have to experiment a little bit more. If you understand a way to do so, let me know!

[piranna]

I got exactly the same problem.

[alexandercerutti]

I might have found a solution to the problem! Sadly, today I didn't have any time to detail it.
Tomorrow I'll open a new issue to discuss this. If you want to open it, be my guest!

[piranna]

I'm honestly interested on knowing about your solution :-)