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

Jquery error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided. #20703

Closed
pmunin opened this issue Dec 14, 2017 · 22 comments
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@pmunin
Copy link

pmunin commented Dec 14, 2017

TypeScript Version: 2.6.2

Code

//package.json:
{
  "name": "issue-ts-isolatedmodule",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "jquery": "^3.2.1"
  }
  ,"devDependencies": {
    "typescript": "^2.6.2",
    "@types/jquery":"^3.2.17"
  }
}

//tsconfig.json

{
  "compilerOptions": {
    "outDir": "build/dist",
    "module": "esnext",
    "target": "es5",
    "lib": ["es6", "dom"],
    "sourceMap": true,
    "allowJs": true,
    "jsx": "react",
    "moduleResolution": "node",
    "rootDir": "src",
    "forceConsistentCasingInFileNames": true,
    "suppressImplicitAnyIndexErrors": true,
    "isolatedModules": true,
    "preserveConstEnums": true,
    "preserveSymlinks": true,
    "allowSyntheticDefaultImports": true
  },
  "exclude": [
    "node_modules",
    "build",
    "scripts",
    "acceptance-tests",
    "webpack",
    "jest",
    "src/setupTests.ts"
  ]
}
//src/index.ts
export function HalloWorld(){
    return "Hallo world!"
}

Executing tsc -p ..

Expected behavior:
successfully compiled /build/dist/{tsfilename}.js for each ts module:

Actual behavior:

node_modules/@types/jquery/index.d.ts(7690,16): error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided.
node_modules/@types/jquery/index.d.ts(7698,16): error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided.
@RyanCavanaugh
Copy link
Member

@mhegazy I was discussing this with Skype earlier this week - it would be much better if const enum was only an error if consumed in an implementation file. Otherwise random .d.ts files from DT can "poison" --isolatedModules compilations. Thoughts?

@pmunin
Copy link
Author

pmunin commented Dec 14, 2017

If it is consumed by a ts file, how to use JQuery types with --isolatedModules on?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 19, 2018

I have created a dtslint issue to ban const enums in declaration files. i think a declaration file in general should be applicable to the strictest settings, and this includes --isolatedModules.

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Jan 30, 2018
@RyanCavanaugh
Copy link
Member

We need to expand the lint rule on DefinitelyTyped as const enum on DT really does not make sense.

Separately, accepting PRs to move the error from a const enum existing to a const enum being referenced (in a value position) when isolatedModules is on. Should be relatively easy.

@zspitz
Copy link
Contributor

zspitz commented Jan 31, 2018

const enum on DT really does not make sense.

@RyanCavanaugh const enum in an ambient context allows documenting a set of values which are not available via the object model, and which are to be inlined on compilation. Instead of this:

const app = new ActiveXObject('Access.Application');
app.RunCommand(97); // what is 97?

the enum values could be defined in the .d.ts file:

declare namespace Access {
    const enum AcCommand {
        // ...
        acCmdSaveRecord = 97,
        // ...
    }
}

and consumed by the .ts file by name:

app.RunCommand(Access.AcCommand.acCmdSaveRecord);

and inlined on compilation.

Ideally, these values should be part of the object model:

// Javascript implementation code
var Access = {
    AcCommand: {
        acCmdSaveRecord: 97
    }
};

and could then be referred to in the .d.ts file as a plain (non-const) enum; but I don't think this is done in practice.

Perhaps this isn't the best mechanism for inlining declared values, as enum doesn't just imply a documented set of values, but also restricting to that set of values. But the compiler doesn't enforce this; the following would compile:

app.RunCommand(-42);

even though -42 is not one of the values used in the enum.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2018

const enum in an ambient context allows documenting a set of fixed values to be inlined on compilation, which are not accessible via the object model. Instead of this:

You should use a union type of literals (string or number) instead.

I understand the documentation rational. but a declaration file is included in multiple projects, and the author of the declaration can not control how the user will build their project. And thus, a .d.ts should adhere to the least common denominator of compiler options. this is not any different from --noImplicitAny or --strictNullchecks.

@zspitz
Copy link
Contributor

zspitz commented Jan 31, 2018

@mhegazy

a .d.ts should adhere to the least common denominator of compiler options

Understood. My question is now, why does --isolatedModules prevent inlining values from const enums?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2018

The flag is meant to support transpiling a single file at a time. i.e. the whole project is not loaded when a file is transformed. and thus the compiler does not have access ot the .d.ts file to find out what values to use.

@zspitz
Copy link
Contributor

zspitz commented Jan 31, 2018

@mhegazy I must be missing something here -- so how can anything from the .d.ts file be enforced?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2018

tsc --isolatedModules processes the whole project. it errors if it is not safe to transpile this project one at a time.

Consider for example using babel to transpile a TS project. you would run tsc as a type checker only, but not as a transpiler. babel only transpiles one file at a time, and has no way to get access to the .d.ts.

hope this explains it.

@zspitz
Copy link
Contributor

zspitz commented Jan 31, 2018

@mhegazy Yes, thanks.

@christophehurpeau
Copy link

I have the same error with chalk and @types/range-parser in node_modules. I cannot change them, and use babel with @babel/preset-typescript, any plans on fixing this ? How can I help ?

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Aug 31, 2018

Same problem with @types/big.js and @babel/preset-typescript

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Aug 31, 2018

Is replacing const emuns by string union types in DefinitelyTyped repo the only way to fix this error?

@ghost
Copy link

ghost commented Nov 7, 2018

@chicoxyzzy Did you find out what's the best practice to handle this problem?

@zspitz
Copy link
Contributor

zspitz commented Nov 7, 2018

@chicoxyzzy @noelyoo You can disable the no-const-enum rule in your tslint.json, like this.

@ghost
Copy link

ghost commented Nov 7, 2018

@zspitz

Disabling the no-const-enum rule does not solve this problem:

Failed to compile.

/Users/noel/react-auth-demo/node_modules/monaco-editor/esm/vs/editor/editor.api.d.ts h-demo/src/containType error: Ambient const enums are not allowed when the '--isolatedModules' flag is provided. TS1209

@alangpierce
Copy link
Contributor

@noelyoo my workaround has been to remove isolatedModules from my config and just be really careful not to use const enums in my project or from third-party libraries, and to set up my build system to compile modules independently anyway.

I think the best long-term solution is #20703 (comment) (making isolatedModules prohibit you from using const enums rather than prohibiting you from using libraries that have a const enum anywhere in their typedefs). Another solution is to be better about getting library authors to publish types that are isolatedModules-friendly, but it seems like there are a lot of const enums in the wild and not a lot of awareness of this issue in the first place.

@ghost
Copy link

ghost commented Nov 9, 2018

@alangpierce @chicoxyzzy I solved the above issue by setting skipLibCheck as true in tsconfig.json based on ianschmitz's suggestion.

facebook/create-react-app#5738

alangpierce added a commit to alangpierce/TypeScript that referenced this issue Nov 10, 2018
…ccess

Fixes microsoft#20703 with solution suggested in microsoft#20703 (comment)

Previously, `--isolatedModules` gave an error for any ambient const enum, which
meant that some third-party libraries would always give errors even if the
ambient const enums they declare were never used. Now, we only give an error
when an ambient const enum is referenced, which allows such libraries to still
be used as long as the const enums are never accessed.

Some nuances:
* As before, the error is only surfaced for *ambient* const enums. With
  non-ambient const enums, we know that an `isolatedModules` build will emit the
  enum and produce a plain reference rather than inlining the constant, so
  everything will still work.
* I originally planned to do this check in the code path that inlines the
  constant, but that code is only exercised at emit time, so, for example, the
  TS language service wasn't giving an error in my editor. Instead, I do the
  check at typecheck time next to another const-enum-related check.
* This can be a breaking change when using `skipLibCheck` because the error is
  typically moved from a .d.ts file to a .ts file.

Testing done:
I ran this TS build on a large project of mine that previously had disabled
`isolatedModules` so I could use the `chalk` library. With `isolatedModules`
enabled, there was no longer an error in the chalk typedefs, and a reference to
the `Level` const enum produced an error in my editor.
@alangpierce
Copy link
Contributor

alangpierce commented Nov 11, 2018

I put up a PR to change the error to happen when accessing an ambient const enum rather than declaring one (#20703 (comment)), which should pretty much make this a non-issue: #28465.

@gsbelarus
Copy link

Hoping it will be available in the next 1.3.7 distributive. This error is showstopper for us.

@wyqydsyq
Copy link

wyqydsyq commented Nov 30, 2018

I was able to resolve this myself by applying the change I made in this PR with patch-package to override the official @types/ package. This approach can be used to work around the issue for other libraries with outdated types too.

buschtoens added a commit to ember-intl/ember-intl that referenced this issue Aug 18, 2020
buschtoens added a commit to ember-intl/ember-intl that referenced this issue Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

10 participants