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

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

Closed
wants to merge 5 commits into from
Closed

Conversation

favna
Copy link
Contributor

@favna favna commented Jul 9, 2023

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"

This was referenced Jul 9, 2023
@favna favna force-pushed the exports branch 2 times, most recently from 65aee2b to fd15c9e Compare July 9, 2023 20:55
@krisk
Copy link
Owner

krisk commented Jul 11, 2023

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
Copy link
Contributor Author

favna commented Jul 11, 2023

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
Copy link
Contributor Author

favna commented Jul 11, 2023

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:
image
image

@krisk
Copy link
Owner

krisk commented Jul 12, 2023

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
Copy link
Contributor Author

favna commented Jul 12, 2023

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
Copy link
Contributor

akphi commented Jul 19, 2023

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

@favna
Copy link
Contributor Author

favna commented Jul 19, 2023

@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.
image

@akphi
Copy link
Contributor

akphi commented Jul 19, 2023

@favna Gotcha. Thank you so much!

package.json Outdated Show resolved Hide resolved
@favna favna marked this pull request as draft August 7, 2023 12:47
@favna favna marked this pull request as ready for review August 7, 2023 13:20
@virtuallyunknown
Copy link

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
Copy link
Contributor Author

favna commented Sep 4, 2023

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
Copy link

jstasiak commented Sep 4, 2023

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
Copy link
Contributor Author

favna commented Sep 4, 2023

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
Copy link
Owner

krisk commented Oct 25, 2023

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

@krisk krisk closed this Oct 25, 2023
@andrewbranch
Copy link

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

@favna
Copy link
Contributor Author

favna commented Oct 25, 2023

@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 f507e9f 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
Copy link
Contributor Author

favna commented Oct 25, 2023

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

@favna
Copy link
Contributor Author

favna commented Oct 25, 2023

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
Copy link
Contributor Author

favna commented Oct 25, 2023

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

image

@favna
Copy link
Contributor Author

favna commented Nov 21, 2023

@krisk when will you pick this up?

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

Successfully merging this pull request may close these issues.

6 participants