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

Imports conditions #551

Merged
merged 11 commits into from
May 15, 2023
Merged

Imports conditions #551

merged 11 commits into from
May 15, 2023

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented May 12, 2023

This implements the design from #542 (comment) under an experimental flag importsConditions. There's still some stuff I probably want to change here (hence why it's under an experimental flag) but I'd like to try it out and see how it is to use first.

A thing I didn't mention in the PR comment linked above that I've done here is for imports fields like this:

{
  "imports": {
    "#a": {
      "worker": "./src/a.js",
      "browser": "./src/b.js",
      "default": "./src/a.js"
    }
  }
}

We'll only generate two bundles for each format rather than three since the worker and default conditions resolve to the same place. (we use the shortest combination of conditions to name bundles)

Also, in the generated .d.ts files, we re-write imports from #something to the resolved thing with the default condition applied. This is so that the .d.ts files work in moduleResolution: node (though of course, when authoring packages, you need to use bundler or nodenext so that #something can be resolved).

In the future, I'm thinking of having essentially two .d.ts outputs, one with #something replaced and one where it's not replaced, this would allow having different types for different conditions. I don't think that by itself would be that useful tbh since I'm guessing that most package consumers will not be configuring customConditions in TS. Though what would be quite useful is the types@semver-range-of-ts-version magical condition thing, specifically, we could essentially polyfill it for moduleResolution: node via typesVersions without users having to deal with typesVersions. One slight problem with this is that without isolatedDeclarations, you could accidentally rely on type information from a particular implementation of #something and therefore generate the wrong .d.ts files so I'm thinking if/when we implement this part, it'll probably stay experimental until isolatedDeclarations exists. (we could of course also just generate .d.ts files for each set of conditions but that would be unacceptably slow imo)

Also, one random thing that's interesting with this design (though I don't think I'd say it's really a problem) is that you can conditionally have different exports for different conditions except that you couldn't conditionally have a default export. (the implementations of a default export could be different for different conditions but you couldn't have a default export under some conditions and not under others)

cc @Andarist

Closes #542

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023

🦋 Changeset detected

Latest commit: 497bf6a

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

@emmatown emmatown marked this pull request as draft May 12, 2023 05:58
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage: 93.95% and project coverage change: +0.01 🎉

Comparison is base (41a8a4f) 91.79% compared to head (56c9856) 91.80%.

❗ Current head 56c9856 differs from pull request most recent head 497bf6a. Consider uploading reports for the commit 497bf6a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
+ Coverage   91.79%   91.80%   +0.01%     
==========================================
  Files          38       41       +3     
  Lines        1730     2002     +272     
  Branches      493      585      +92     
==========================================
+ Hits         1588     1838     +250     
- Misses        133      154      +21     
- Partials        9       10       +1     
Impacted Files Coverage Δ
packages/cli/src/build/utils.ts 100.00% <ø> (ø)
packages/cli/src/project.ts 85.13% <ø> (ø)
packages/cli/src/validate.ts 88.23% <ø> (ø)
packages/cli/test-utils/index.ts 96.31% <81.81%> (-1.09%) ⬇️
packages/cli/src/build/config.ts 78.94% <83.33%> (-2.22%) ⬇️
...eclarations/get-used-declaration-with-replacing.ts 85.18% <85.18%> (ø)
packages/cli/src/package.ts 95.42% <92.85%> (-0.38%) ⬇️
packages/cli/src/imports.ts 95.19% <95.19%> (ø)
packages/cli/src/utils.ts 98.62% <95.74%> (-1.38%) ⬇️
packages/cli/src/dev.ts 95.53% <96.07%> (-0.26%) ⬇️
... and 10 more

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

@emmatown emmatown force-pushed the imports-conditions branch from a163550 to 483cd76 Compare May 12, 2023 06:23
emmatown added 4 commits May 12, 2023 16:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@emmatown emmatown marked this pull request as ready for review May 15, 2023 00:35
@emmatown emmatown merged commit ff61fbf into main May 15, 2023
@emmatown emmatown deleted the imports-conditions branch May 15, 2023 00:46
@github-actions github-actions bot mentioned this pull request May 15, 2023
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.

None yet

1 participant