Skip to content

Fix importConditionDefaultExport - #546

Merged
emmatown merged 2 commits into
mainfrom
fix-importConditionDefaultExport
May 2, 2023
Merged

emmatown merged 2 commits into
mainfrom
fix-importConditionDefaultExport

Conversation

@emmatown

@emmatown emmatown commented May 2, 2023

Copy link
Copy Markdown
Member

The previous version didn't work with "moduleResolution": "bundler", this fixes that.

@changeset-bot

changeset-bot Bot commented May 2, 2023 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ae70567

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 Patch

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 Bot commented May 2, 2023 •

Copy link
Copy Markdown

Codecov Report

Patch coverage: 94.11% and project coverage change: +2.95 🎉

Comparison is base (e854fe2) 88.70% compared to head (68334cc) 91.66%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
+ Coverage   88.70%   91.66%   +2.95%     
==========================================
  Files          37       37              
  Lines        1664     1691      +27     
  Branches      465      474       +9     
==========================================
+ Hits         1476     1550      +74     
+ Misses        180      132      -48     
- Partials        8        9       +1     
Impacted Files Coverage Δ
packages/cli/src/dev.ts 95.74% <83.33%> (+8.68%) ⬆️
packages/cli/src/rollup-plugins/mjs-proxy.ts 100.00% <100.00%> (+100.00%) ⬆️
...rc/rollup-plugins/typescript-declarations/index.ts 95.16% <100.00%> (+7.02%) ⬆️
packages/cli/src/utils.ts 100.00% <100.00%> (+17.89%) ⬆️

... and 5 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.

@emmatown
emmatown requested a review from Andarist May 2, 2023 04:38
Comment thread packages/cli/src/dev.ts
Comment on lines +290 to +295
// the * won't really do anything right now
// since cjs-module-lexer won't find anything
// but that could be fixed by adding fake things
// to the .cjs.js file that look like exports to cjs-module-lexer
// but don't actually add the exports at runtime like esbuild does
// (it would require re-running dev when adding new named exports)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand what's happening here but it sounds like it does nothing today so that's... fine? 😅

@emmatown emmatown May 2, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

correct (this bit is also not new to this PR, it's just moved around)

The comment is generally about making this work in preconstruct dev:

// somewhere.mjs
import { something } from 'my-pkg'

It currently doesn't (and didn't before any of the importConditionDefaultExport stuff).

@Andarist Andarist left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I understand correctly, the core of this PR is to avoid loading CJS file directly into MJS because TS doesn't understand module but yet it understands import and changes what kind of a default we get there (tangent... is this even OK? i guess that it might be because it kinda acts as module here even though not through that condition, OTOH... this isn't how strict bundlers work, I think). To avoid this we create an extra CJS file to normalize the default thing so it can mean the same thing for the runtime and types in more scenarios

@emmatown

emmatown commented May 2, 2023

Copy link
Copy Markdown
Member Author

Yes, your understanding is correct though it's specifically about the default, we do import the .cjs.js directly from the .cjs.mjs for the named exports.

@emmatown
emmatown merged commit c28b10a into main May 2, 2023
@emmatown
emmatown deleted the fix-importConditionDefaultExport branch May 2, 2023 08:07
@github-actions github-actions Bot mentioned this pull request May 2, 2023
@emmatown
emmatown restored the fix-importConditionDefaultExport branch November 22, 2025 01:17
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