Skip to content

add ESM exports in package.json - #692

Closed
akphi wants to merge 2 commits into
krisk:mainfrom
akphi:exports
Closed

add ESM exports in package.json#692
akphi wants to merge 2 commits into
krisk:mainfrom
akphi:exports

Conversation

@akphi

@akphi akphi commented Oct 3, 2022 •
edited
Loading

Copy link
Copy Markdown

@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
Jack-Works
Jack-Works reviewed Nov 4, 2022
View reviewed changes
Comment thread package.json Outdated
Comment on lines +16 to +18
"types": "./dist/fuse.d.ts",
"import": "./dist/fuse.esm.js",
"require": "./dist/fuse.common.js"

@Jack-Works Jack-Works Nov 4, 2022

Copy link
Copy Markdown

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

@akphi akphi Jul 9, 2023

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

@Jack-Works Jack-Works mentioned this pull request Nov 4, 2022
Closed
@rchandu

rchandu commented Mar 28, 2023

Copy link
Copy Markdown

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

@akphi

akphi commented Mar 28, 2023

Copy link
Copy Markdown
Author

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

favna
favna suggested changes Jul 9, 2023
View reviewed changes
Comment thread 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"
}
},

@favna favna Jul 9, 2023

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

@akphi akphi Jul 9, 2023

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

@favna favna Jul 9, 2023

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

akphi commented Jul 9, 2023

Copy link
Copy Markdown
Author

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

@favna

favna commented Jul 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

favna commented Jul 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

akphi commented Jul 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

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 favna mentioned this pull request Jul 9, 2023
Closed
@favna

favna commented Jul 9, 2023

Copy link
Copy Markdown
Contributor

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

Reviewers

2 more reviewers
@favna favna favna requested changes
@Jack-Works Jack-Works Jack-Works left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@akphi @rchandu @favna @Jack-Works