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: mark names field in ESMExport as possibly undefined #273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tobiasdiez
Copy link

The names field in ESMExport could be undefined as (indirectly) has been reported in nuxt/module-builder#309.

This is now properly reflected in the types. Moreover, the regex group matches are now typed correctly.

tobiasdiez added a commit to tobiasdiez/module-builder that referenced this pull request Aug 13, 2024
`names` can be null with star exports. Upstream fix to improve ts: unjs/mlly#273

Fixes nuxt#309
@pi0
Copy link
Member

pi0 commented Oct 6, 2024

Thanks for PR and sorry for late reply. I love the internal type util for regexes!

However, considering the types already declare names as existing, i think we might normalize it to an empty array inside normalizeExports wdyt?

@tobiasdiez
Copy link
Author

Thanks for PR and sorry for late reply. I love the internal type util for regexes!

No worries!

However, considering the types already declare names as existing, i think we might normalize it to an empty array inside normalizeExports wdyt?

I don't have a strong opinion on this. I guess you can make a case for both: returning an empty array would be slightly easier to use, while making names optional aligns more with the other optional fields and may better convey that the result is not always available / makes sense.

Let me know what you prefer and I'll implement it.

@pi0
Copy link
Member

pi0 commented Oct 7, 2024

I think mainly for the sake of not breaking current deps (that assume names is not nullable based on types) it would be a faster solution to set an empty array.

I agree about benefits of making it optional, perhaps we can do it in next major versions.

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