fix: add proper ESM exports in package.json

favna commented 11 months ago

This builds on top of and resolves #692. As #692 needed more work done but also first had to be rebased on main it was decided to supersede the PR with this one as I already did the rebase. First the body from that PR as written by @akphi:

@krisk We're using Typescript module: Node16 in our project and since Fuse.js ESM does not specify type: module and exports field in package.json, it does not get picked up properly by Typescript. So I would like to add these to the package.json file.

The error I got is something like this, for the following import:
import Fuse from 'fuse.js';

const searchEngine = new Fuse(...

^
This expression is not constructable.
  Type 'typeof import("/Users/user/my-project/node_modules/fuse.js/dist/fuse")' has no construct signatures.ts(2351)
I believe this would help with #541 as I have noted there

For Typescript esm mode, such as Node16, NodeNext, esModuleInterop is not supported in this mode, it's worth if we do something like lukeed/clsx#44 - I have given it an attempt but not quite sure how I can modify the d.ts file accordingly.

I have tested all the following scenarios in a project that uses fuse.js as a dependency. When talking about "type" this is referring to the field in package.json, when talking about "module" and "moduleResolution", these refer to the fields in tsconfig.json.

JavaScript based projects
- "type": "common"
- ✅ .cjs file extension
- ✅ .mjs file extension
- ✅ .js file extension
- "type": "module"
- ✅ .cjs file extension
- ✅ .mjs file extension
- ✅ .js file extension
TypeScript based projects
- "module": "Node16", "moduleResolution": "Node16"
  - ✅ "type": "module"
  - ✅ "type": "commonjs"
- "module": "Node16", "moduleResolution": "Node"
  - ✅ "type": "module"
  - ✅ "type": "commonjs"
- "module": "CommonJS", "moduleResolution": "Node"
  - ✅ "type": "module"
  - ✅ "type": "commonjs"

krisk commented 11 months ago

Thanks for this! I had never been a fan of changing extensions, but that was way back in the day, and times have changed 😄 Perhaps then it's also worth simply removing common|esm from the filenames as well:

*.common(.min).cjs -> *.cjs
*.esm(.min).mjs -> *.mjs

Thoughts?

favna commented 11 months ago

Yeah I can make that change, the extension implies the type anyway.

As for the whole changing extensions and such, trust me I'm not a fan either but sadly as long as CJS is still widely used in the Node ecosystem there is no real other way to ensure proper support for all setups :(

favna commented 11 months ago

Also found that I made a mistake with the regex for prod detection for minify so that's also fixed now.

Fancy btw, because of the renames and with VSCode file nesting options:

{
  "explorer.fileNesting.enabled": true,
  "explorer.fileNesting.expand": false,
  "explorer.fileNesting.patterns": {
    "*.d.ts": "$(capture).d.ts.map",
    "*.js": "$(capture).js, $(capture).js.map, $(capture).min.js, $(capture).cjs, $(capture).cjs.map, $(capture).mjs, $(capture).mjs.map, $(capture).global.js",
    "*.ts": "$(capture).js, $(capture).js.map, $(capture).min.js, $(capture).d.ts, $(capture).d.ts.map, $(capture).cjs, $(capture).mts, $(capture).cts, $(capture).cjs.map, $(capture).mjs, $(capture).mjs.map, $(capture).global.js, $(capture).scss, $(capture).spec.ts, $(capture).stories.ts, $(capture).html",
    "*.tsx": "$(capture).js"
  },
}

It nests most of the files in dist:

krisk commented 11 months ago

Ok just got around to testing. Lots of changes here. I'll go ahead and also update the docs. Note that this is likely to be a breaking change, so I may issue a major version with a pre-release tag (i.e v7.0.0-0)

favna commented 11 months ago

Note that this is likely to be a breaking change

Hmm.. I'll let you make that call. I don't think it's a breaking change per se, but I suppose if end-users expect the dist files to have certain names as opposed to relying on the package.json module resolution then yeah it would be breaking. There is definitely an argument that can be made in favour of breaking.

Either way, to be frank, I don't personally use Fuse anymore (in favour of a custom fuzzy search with the Jaro Winkler algorithm) so all good here. I just keep tabs on the library from time to time as with my earlier PR because I think the lib is really cool and useful.

akphi commented 11 months ago

@favna just curious, which library do you use for fuzzy search using Jaro Winkler algorithm?

favna commented 11 months ago

@favna just curious, which library do you use for fuzzy search using Jaro Winkler algorithm?

Lib that was primarily written by a friend of mine and some help from my side (mostly on the ops stuff though): https://github.com/skyra-project/jaro-winkler

And this is how it is actually used: https://github.com/favware/graphql-pokemon/blob/main/src/lib/utils/FuzzySearch.ts

So it's quite a bit more involved than Fuse because the lib only calculates the distances and that's it but for my use case, I prefer that Jaro Winkler algorithm gives more consistent results. For example, I can give it !@#@!#}{}{ASdASD123-912-3jklsadksad@!#@!}#|{}"" and still get some results, which as silly as it may be, for my particular use case is what I do want. For the record, that weird ass string gives this as a result. Don't ask me how I'll leave that up to mathematicians like Misters Jaro and Winkler.

akphi commented 11 months ago

@favna Gotcha. Thank you so much!

virtuallyunknown commented 10 months ago

Hey folks, I don't mean to be pushy but is this release and fix coming out soon? At the moment I can't use the library.

This works in runtime but not in compile time:

import fuse from 'fuse.js';
new fuse([]);

And vice-versa:

import fuse from 'fuse.js';
new fuse.default([]);

favna commented 10 months ago

FWIW you can for now squash the branch into 1 patch file then use it through patch-package (npm) or yarn/pnpm patches (built in to those)

But yeah I hope krisk can merge and release this soon.

jstasiak commented 10 months ago

This is what we do in ESM code – it builds and runs:

import FuseModule from 'fuse.js'
const Fuse = FuseModule as unknown as typeof FuseModule.default
const fuseInstance = new Fuse(...)

favna commented 10 months ago

I generated the patchfile I mentioned, but I also realised while looking at it that it probably wont work because it applies to the whole repo, not only what is published to npm. That is to say, it also includes changes to for example GitHub Actions workflows.

patchfile.patch

krisk commented 8 months ago

Had to manually merge and make some changes, but this is fixed as of v7

andrewbranch commented 8 months ago

The types are still broken for anyone not using ESM: https://arethetypeswrong.github.io/?p=fuse.js%407.0.0

favna commented 8 months ago

@krisk you essentially merged #692 instead of this PR which is why @andrewbranch's report is in fact correct that Fuse is currently broken for CJS users.

I would recommend rolling back to commit f507e9f19ff022c13e4517ecff3e7a72ee2688ee and then re-applying this PR then releasing 7.0.1 ASAP (you'll probably want to run npx standard-version -a -r patch manually as opposed to through your release script)

favna commented 8 months ago

I applied the same changes you had to apply onto this branch so you can take the appropriate steps more easily @krisk

favna commented 8 months ago

Made it even easier - rebased on the main branch and resolved all conflicts so if you re-open this PR then you should be able to merge it right away.

favna commented 8 months ago

FWIW after merging this PR the issue will be resolved as was always intended:

favna commented 7 months ago

@krisk when will you pick this up?

krisk / Fuse

fix: add proper ESM exports in package.json #727