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

Some tsconfig.json options are unintentionally overridden which can result in non-obvious TS compiler errors #4829

Closed
justinvp opened this issue Jun 15, 2020 · 4 comments · Fixed by #7068
Labels
kind/enhancement Improvements or new features language/javascript resolution/fixed This issue was fixed

Comments

@justinvp
Copy link
Member

justinvp commented Jun 15, 2020

We use ts-node when running Node.js Pulumi programs and policy packs.

// We provide reasonable defaults for many ts options, meaning you don't need to have a tsconfig.json present
// if you want to use TypeScript with Pulumi. However, ts-node's default behavior is to walk up from the cwd to
// find a tsconfig.json. For us, it's reasonable to say that the "root" of the project is the cwd,
// if there's a tsconfig.json file here. Otherwise, just tell ts-node to not load project options at all.
// This helps with cases like pulumi/pulumi#1772.
const skipProject = !fs.existsSync("tsconfig.json");
if (typeScript) {
tsnode.register({
typeCheck: true,
skipProject: skipProject,
compilerOptions: {
target: "es6",
module: "commonjs",
moduleResolution: "node",
sourceMap: "true",
},
});
}

The same code is duplicated for policy packs here (consolidating is tracked by pulumi/pulumi-policy#48).

We provide some reasonable default values for when tsconfig.json doesn't exist. Unfortunately, the way this is written causes the specified defaults to override any values set in the actual tsconfig.json file. So if target is set to es2016 in tsconfig.json, the value that ends up being used is es6.

This can lead to non-obvious TypeScript compiler errors, such as error TS2339: Property 'includes' does not exist on type 'string[]', which would work if targeting es2016, but doesn't work when targeting es6 (when @types/node is not installed). (A workaround in this particular case would be to include @types/node as a dependency. Or set lib to be es2016 so that TS will use the ES2016 type declaration library, even though its codegen is targeting ES6).

We should fix this to only set the default values when tsconfig.json doesn't exist. (For compatibility, we may actually want to load tsconfig.json and set these default values if they aren't defined in tsconfig.json.)

Also, related to pulumi/templates#116. As part of fixing this, we should update the default target to es2018, which has broad support in Node 10+ (all the versions of Node we support).

@leezen
Copy link
Contributor

leezen commented Jun 15, 2020

Note that this is documented behavior per https://www.pulumi.com/docs/intro/languages/javascript/#disabling-built-in-typescript-support and if we do update the ES target, we should also update the above documentation.

@justinvp
Copy link
Member Author

this is documented behavior

And looks intentional per #1569 (comment):

We set the following compiler options (this is what we set in our existing tsconfig.json's):

compilerOptions: {
  target: "es6",
  module: "commonjs",
  moduleResolution: "node",
  sourceMap: "true",
},

Any additional options in a tsconfig.json will be merged in. If you want to change one of the above, you'll have to fall back to using tsc directly, since ts-node prefers the options we explicitly set to ones in tfconfig.json.

But definitely worth revisiting whether we want to continue to force es6.

@leezen
Copy link
Contributor

leezen commented Jun 16, 2020

Definitely agree it's worth revisiting. It's not clear to me why we need to force es6

@tmccombs
Copy link

I think this might be a duplicate of #2673 (although there is more information on this issue).

And looks intentional per

I'm not sure if that was intention though, or just a side-effect of passing options in case tsconfig.json is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features language/javascript resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants