Skip to content
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

Readme compatibility note about makeTokenizer #1351

Open
farski opened this issue Dec 2, 2024 · 5 comments
Open

Readme compatibility note about makeTokenizer #1351

farski opened this issue Dec 2, 2024 · 5 comments

Comments

@farski
Copy link

farski commented Dec 2, 2024

Might be good to add a small note under the Compatibility section of the readme about makeTokenizer going away. It's not a hard change to resolve once you find the error, but without a changelog it's not obvious that work will be needed when upgrading.

@Borewit
Copy link
Owner

Borewit commented Dec 2, 2024

Sorry about that. and thanks for reaching out @farski . There is a sort of change log available: Releases.

Version v0.5.0 explicitly highlights an API change.

Additionally, in the PR itself: #1341, the changes are documented.

I agree, it requires some digging to piece everything together. The compatibility section serves a slightly different purpose; it might help to have a "prerequisites" section to outline what's needed to use this module.

Also, since the version starts with 0, it's a clear indicator that this is still a work in progress, and the API is subject to change.

@Borewit
Copy link
Owner

Borewit commented Dec 2, 2024

Totally off-top here @farski, but are you still interested in: sindresorhus/file-type#646?

It's follow up on your issue, to allow a configurable tolerance of noise.

@farski
Copy link
Author

farski commented Dec 2, 2024

I guess I hadn't done maintenance on this app for longer than I thought, because it was still back on 2.3, so I didn't realize how many versions it was upgrading.

An aside: ESLint 8 has no problem with 2.3, but 3+ it gives an error Unable to resolve path to module when importing. I tried some things to make ESLint happy, but the only thing I found that worked was adding main back to package.json for the local @tokenizer/s3 package.

Re: noise tolerance, that PR feels like a very specific fix, and I wouldn't want it to be added just for our needs. Even that would only solve one particular file detection issue we have found, which isn't even the only common problem with MP3s that sometimes prevents proper file type detection, much less other file types.

The original request is still what we'd be looking for. Custom detectors would allow us to add in more and more custom behavior over time, as we find common data issues with files coming through our system that prevent standard file type detection. We want to be able to add custom behaviors in a way that is compatible with the types of files we deal with, even if they are sort of sketchy and don't provide results that are reliable enough to merge upstream. I had started on work to add that functionality to file-type a while back, I could pick it up again if you're interested.

@Borewit
Copy link
Owner

Borewit commented Dec 3, 2024

An aside: ESLint 8 has no problem with 2.3, but 3+ it gives an error Unable to resolve path to module when importing. I tried some things to make ESLint happy, but the only thing I found that worked was adding main back to package.json for the local @tokenizer/s3 package.

You mean migrating from version 0.2.3 to 0.3.0? Which was going from CommonJS to ESM. So you had to make the the ESM package look like an CommonJS, to keep ESLint happy? The main property has not been forgotten, it does long belong their. Maybe check your ESLint config or try something easier to use, like Biome.

I will think about an option where to inject a detector in file-type.

@Borewit
Copy link
Owner

Borewit commented Dec 9, 2024

@farski, check load-esm for loading pure ESM modules in a TypeScript / CommonJS project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants