search

sindresorhus / file-type

Detect the file type of a file, stream, or data
MIT License
3.72k stars 354 forks source link

Add support for WebVTT #658

Closed AleksandrHovhannisyan closed 2 months ago

AleksandrHovhannisyan commented 3 months ago

Resolves: https://github.com/sindresorhus/file-type/issues/657

Docs/magic numbers: https://www.w3.org/TR/webvtt1/#iana-text-vtt

Borewit commented 3 months ago

@AleksandrHovhannisyan, I propose to keep the single fixture as you got it from WRC and drop the reverse engineered fixtures to aiming to match each signature cases. Keep it simple.

AleksandrHovhannisyan commented 3 months ago

@Borewit Okay, happy to do that, although I will say I found the fixtures helpful when writing the code, as I wrote those first and ran tests as I made adjustments. Without those fixtures, there's no way to guarantee that the code works for edge cases.

Also, I think it would help both of us if I could get a single set of change requests for this PR. Happy to push whatever changes are needed to get it merged, but I do want to limit back and forth.

AleksandrHovhannisyan commented 2 months ago

Hmm, not sure why tests are failing for Node 20+. Node 18 works fine locally for me.

What is the procedure for running those same CI tests locally? npm run test only runs on my local Node 18.

Borewit commented 2 months ago

What is the procedure for running those same CI tests locally?

You essentially need to install a different version of Node or use some fancy mechanism to switch Node versions.

Some tests are only performed on Node >= 20

AleksandrHovhannisyan commented 2 months ago

Okay that's fine, I have nvm/fnm so I'll just use one of those. Thanks