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: package exports and type definitions #8

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

aryaemami59
Copy link
Contributor

This PR:

  • Fixes package exports and type definitions.
  • Migrates to NodeNext module resolution.

Before:

fuzzysearch-before

After:

fuzzysearch-after

@m31coding
Copy link
Owner

Hi @aryaemami59,

Thank you very much for this PR. I am not a node expert and I would like to better understand and reproduce the issues. Which tool / command did you use to create the console output in the before / after screenshots?

Best regards,
Kevin

@aryaemami59
Copy link
Contributor Author

@m31coding I used Are The Types Wrong, it's a very good tool, I also added it to the CI workflow.

You can run it like this:

npx @arethetypeswrong/cli@latest --pack

"types": "./dist/fuzzy-search.d.ts",
"default": "./dist/fuzzy-search.modern.js"
".": {
"module-sync": {
Copy link
Owner

Choose a reason for hiding this comment

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

How is "module-sync" used, what uses it? I have not seen this before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a new exports condition that allows for synchronously requiring an ESM file within CJS files.

docs

package.json Outdated
},
"scripts": {
"test": "jest --coverage",
"dev": "microbundle watch",
"build": "microbundle",
"build": "microbundle && cp dist/fuzzy-search.d.ts ./dist/fuzzy-search.d.cts",
Copy link
Owner

Choose a reason for hiding this comment

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

I think this will not work on Windows. We could install cpx as a development dependency and use it instead of cp, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it doesn't work on Windows. I fixed it in my latest commit if you wanna take a look.

@m31coding m31coding merged commit da16d5e into m31coding:main Nov 22, 2024
1 check passed
@m31coding
Copy link
Owner

@aryaemami59 PR is merged, thank you again for your contribution, very much appreciated!

Best regards,
Kevin

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.

2 participants