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

preserveValueImports + isolatedModules -> type-only ambient const enums #47817

Closed
wants to merge 1 commit into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Feb 9, 2022

Currently, if you compile import Big, { RoundingMode } from "big.js" with preserveValueImports and isolatedModules the import name is preserved and you get at runtime:

import Big, { RoundingMode } from "big.js";
              ^^^^^^^^^^^^
SyntaxError: Named export 'RoundingMode' not found. The requested module 'big.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'big.js';
const { RoundingMode } = pkg;

RoundingMode is an ambient const enum, defined in @types/big.js. There's no corresponding runtime value in big.js.

This PR extends the existing preserveValueImports + isolatedModules requirement, that imported types must be marked type-only, to ambient const enums.

If you add the type modifier (import Big, { type RoundingMode } from "big.js"), the compiler does remove the import name, solving the runtime error.

This PR specifically excludes non-ambient const enums, because they don't lead to runtime errors. (The import name is preserved, but the const enum is also preserved, so not an error at runtime.)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 9, 2022
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@sandersn
Copy link
Member

Possibly related to #46556 and #46626?

@jablko can you open an issue for discussion of this PR too? My initial reaction is that, in the PR you mentioned, the maintainers of @types/big.js acknowledged that RoundingMode was a mistake and would have removed it if the package were less depended on. I'm not sure changing the compiler is the right thing here.

@sandersn sandersn self-assigned this Feb 24, 2022
@sandersn sandersn added this to Not started in PR Backlog via automation Feb 24, 2022
@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Feb 24, 2022
@jablko
Copy link
Contributor Author

jablko commented Feb 25, 2022

@sandersn My rationale is:

  • A compile-time error would be better than a runtime one?
  • If isolatedModules is on, there's no difference to consumers between a type and an ambient const enum, so I think the proposed change to the predicate is correct?

I've opened #48040 for this.

It's related to #46626 in that deconstified enums (whether via that PR or a build step) are independent of this PR, but that's not as important as the other deconstification reasons (JS consumers, single-file transpilation, compile-time vs. runtime version compatibility).

This PR deals with ambient const enums when isolatedModules is on vs. #46556 deals with const enums when they're inlined, i.e. off --- whether type-only const enums should act like single-file transpilation ambient const enums (no value usage --- the current behavior) vs. .d.ts const enums (inlined --- the proposed behavior).

@sandersn
Copy link
Member

To help with PR housekeeping, I'm going to close this PR while it's still waiting on its bug to be accepted.

@sandersn sandersn closed this Mar 15, 2022
PR Backlog automation moved this from Waiting on author to Done Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants