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 incorrect default export type as mentioned in issue #33 #38

Merged

Conversation

atombrenner
Copy link
Contributor

As mentioned in Issue #33 , the typing in index.d.ts incorrectly declared a default export, but index.js has no default export. This caused issues with Node16/CommonJS module settings and having esModuleInterop set to false. (More details.)
This fix deliberately changes only the type definition (to be exact, the generator for the type definition).
Because the package also exports types, the fix suggested in #33 was not enough. To be able to export types with the old syntax, I had to add a namespace declaration in which the exported types are nested.

As a drive-by change, the tsd dependency (tool for testing type definitions) was also updated to the newest version.

This change might break clients that applied one of the suggested workarounds in issue #33 as the types are now correct.
Clients not affected should see no changes.

Copy link
Collaborator

@lovell lovell left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the PR. As you say, this looks like a breaking change, but the workaround appears to include the use of as unknown so I think we're OK for a patch version. Will wait for feedback from others before merging.

@atombrenner
Copy link
Contributor Author

@lovell are we good to merge and release now?

@lovell lovell merged commit cc06c85 into devongovett:master Jan 14, 2025
1 check passed
@lovell
Copy link
Collaborator

lovell commented Jan 14, 2025

2.0.2 now available with this fix, thank you.

paescuj added a commit to directus/directus that referenced this pull request Mar 1, 2025
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