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

add ESM exports in package.json #692

Closed
wants to merge 2 commits into from
Closed

add ESM exports in package.json #692

wants to merge 2 commits into from

Conversation

akphi
Copy link
Contributor

@akphi akphi commented Oct 3, 2022

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

@akphi akphi marked this pull request as ready for review October 3, 2022 17:26
package.json Outdated
Comment on lines 16 to 18
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"
"import": {
"types": "./dist/fuse.d.ts",
"default": "./dist/fuse.esm.js"
},
"require": {
"types": "./dist/fuse.d.ts",
"default": "./dist/fuse.common.js"
}

see microsoft/TypeScript#50466

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated! Thanks for reviewing!

@rchandu
Copy link

rchandu commented Mar 28, 2023

This would be something that would help us. Is there something I can do to help here on this effort ?

@akphi
Copy link
Contributor Author

akphi commented Mar 28, 2023

@rchandu seems like there hasn't been active development in this repo for a couple of months now 🤔

package.json Outdated
Comment on lines 13 to 20
"type": "module",
"exports": {
".": {
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"type": "module",
"exports": {
".": {
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"
}
},
"exports": {
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"
},

The old version was invalid. You cannot just add "type": "module" unless Fuse itself is rewritten to ESM. Also "type": "module" only affects the module itself, not when the module is loaded through node_modules.

Furthermore the subgrouping of "." is not necessary because Fuse doesn't have subpath exports

Lastly, after including this change thus reverting "type": "module" you should also revert the other .cjs related changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! I have updated to your suggestions.

It's been a while and I haven't checked again, locally, but I hope this would work well with Typescript + Jest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should. I maintain the Sapphire project and we use this structure in all projects there are it works with a variety of stacks. CJS, ESM, Jest, Vitest, etc

@akphi
Copy link
Contributor Author

akphi commented Jul 9, 2023

@krisk Could you kindly please take a look at this? Thanks!

@favna
Copy link
Contributor

favna commented Jul 9, 2023

@akphi expect a PR to your fork with a completely refreshed but also thoroughly tested setup very soon, when you merge that it'll pop up here for Krisk to then review fully.

I noticed my comment above introduced some other issues as well but now I am testing the scenarios:

  • CJS (no fluff, super basic package.json)
  • MJS (with type module and/or mjs file extension)
  • TypeScript with "module": "CommonJS" and "moduleResolution": "Node"` (essentially legacy CJS)
  • TypeScript with "module": "Node16" and "moduleResolution": "Node"` (essentially modern CJS)
  • TypeScript with "module": "Node16" and "moduleResolution": "Node16"` (essentially modern ESM)

Edit: Only having some trouble with the last one still so I need to figure that out.

@favna
Copy link
Contributor

favna commented Jul 9, 2023

@akphi I created akphi#1 just now but before it can be merged you need to rebase your own exports branch on top of Krisk/master. Alternatively I could create my own PR targeting Krisk/master from my own fork and we close this PR but that means your contribution credit is lost, your call.

@akphi
Copy link
Contributor Author

akphi commented Jul 9, 2023

hey @favna, thanks for being super transparent; I really don't mind you carrying that out; all I need is just to have ESM ready :) Thanks so much for the help! - let me know, I'll close it out in favor of your PR

@favna
Copy link
Contributor

favna commented Jul 9, 2023

I have created #727 so this PR can be closed @akphi

@akphi akphi closed this Jul 9, 2023
@akphi akphi deleted the exports branch July 9, 2023 20:49
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.

None yet

4 participants