Skip to content

fix: mark names field in ESMExport as possibly undefined#273

Open
tobiasdiez wants to merge 1 commit into
unjs:mainfrom
tobiasdiez:fix-names
Open

fix: mark names field in ESMExport as possibly undefined#273
tobiasdiez wants to merge 1 commit into
unjs:mainfrom
tobiasdiez:fix-names

Conversation

@tobiasdiez

Copy link
Copy Markdown

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

pi0 commented Oct 6, 2024

Copy link
Copy Markdown
Member

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

pi0 commented Oct 7, 2024

Copy link
Copy Markdown
Member

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