Fixed the default export shape in strict ESM environments

[Andarist]

The time has come, brace yourself, yadayada 😉

[changeset-bot]

🦋 Changeset detected

Latest commit: a863d53

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@preconstruct/cli	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.66 🎉

Comparison is base (bb10c5d) 90.64% compared to head (cf93acb) 91.30%.

❗ Current head cf93acb differs from pull request most recent head a863d53. Consider uploading reports for the commit a863d53 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
+ Coverage   90.64%   91.30%   +0.66%     
==========================================
  Files          36       37       +1     
  Lines        1592     1667      +75     
  Branches      452      468      +16     
==========================================
+ Hits         1443     1522      +79     
+ Misses        141      137       -4     
  Partials        8        8              

Impacted Files	Coverage Δ
packages/cli/src/build/rollup.ts	92.85% <100.00%> (+0.31%)	⬆️
packages/cli/src/build/utils.ts	100.00% <100.00%> (ø)
packages/cli/src/dev.ts	97.64% <100.00%> (+0.34%)	⬆️
packages/cli/src/package.ts	95.89% <100.00%> (+1.18%)	⬆️
packages/cli/src/project.ts	85.13% <100.00%> (+5.50%)	⬆️
packages/cli/src/rollup-plugins/mjs-proxy.ts	100.00% <100.00%> (ø)
...rc/rollup-plugins/typescript-declarations/index.ts	94.91% <100.00%> (+0.57%)	⬆️
packages/cli/src/utils.ts	100.00% <100.00%> (ø)
packages/cli/test-utils/index.ts	97.40% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Andarist]

I'd love to use this repo-wide, see a similar concern+question here

[emmatown]

Yes to making it project-wide, an empty object should be valid and equivalent to true (i would imagine an empty object already works on a package)

[emmatown]

The proxy files need to be written for preconstruct dev as well (let’s just do an export * from there)

[Andarist]

what about declaration maps and other source map things? i'm not sure how to handle those correctly

[emmatown]

We only generate js source maps for UMD builds so that isn’t a thing. For declaration maps, you should be able to just write the same declaration map we use for .cjs.d.ts but as .cjs.d.mts

[emmatown]

Thinking about this more and the fact that I think the right answer is just to avoid default exports, I’d like to have three modes:

• the current "you need to do .default" thing
• what this PR implements with an import condition
• no import condition but fail builds which have default exports in entrypoints

For the config, probably this:

"importConditionDefaultExport": "namespace" |  "default" | "namespace-disallow-default"

It would be outside of the exports config. If you set it to default when exports isn't enabled, that would be an error. For now, it would default to namespace. In the next major, it would default to namespace-disallow-default.

[Andarist]

While there are good reasons to avoid default exports, I don't think that it should be "mandated" by a tool. Different people like different things and default might even sometimes be required by some frameworks for different purposes.

I also believe that the problem is not even exclusive to packages with default export. Even without default this is still allowed:

import ns from 'cjs-lib'

I think that's not a good thing because it can accidentally be used by users and it's likely not how the lib's usage is documented.

[emmatown]

Yeah, fair. I was initially thinking who cares about the namespace being technically available but i suppose if more people actually use native ESM, it'll be kinda annoying so sure.

I do think I prefer the config with string values instead of booleans though, so if you could make the config this (still inside of exports unlike my last comment suggests though + allow it to be specified at a project level exports option), that would be great.

"importConditionDefaultExport": "namespace" |  "default"

[Andarist]

is this because TS already supported import in declaration files, it had its own semantics and stuff so now it can't use that syntax alone to differentiate between loading a thing with import/require semantics? 🤔

[emmatown]

yes

[emmatown]

Re naming bike shedding: curious to hear reasoning behind the naming you've done?

I've added some docs using the naming I suggested. To me, importConditionDefaultExport makes more sense than importDefaultExport since it's specifically about the import condition and it doesn't affect importing the default export with ESM syntax in files that are not type: module/.mjs/etc. And I don't really see what the unwrapped- in unwrapped-default adds given the alternative value is namespace.