-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[sdk/nodejs] Prevent Pulumi from overriding tsconfig.json options. #7068
Conversation
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR |
Does this also address #3061? |
@justinvp Any thoughts on this? I seem to recall us having some tests for the tsnode compilation and configuration, but can't see where they are currently. |
|
||
if (opts.typeScript) { | ||
tsnode.register({ | ||
typeCheck: true, | ||
skipProject: skipProject, | ||
skipProject: true, | ||
compilerOptions: { | ||
target: "es6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that since the default Pulumi templates use target: "es2016"
, this PR will lead to a change of beaviour for ~all users, where their tsnode compilation starts targeting es2016. I believe that should be fine (and indeed likely good). But it is a change, so we should be sure to do some further testing of this (for example, a run of the examples
repo) to see if there are any issues raised.
Also related to pulumi/templates#116 which would involve bumping up the default target for new projects, which after this change would (correctly, I believe) cause those changes to get picked up by the tsnode compilation.
pulumi/sdk/nodejs/cmd/run-policy-pack/run.ts Lines 168 to 177 in 9abcca3
This part does not override the |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from @lukehoban's comments.
I’m looking forward to this change! Just today I was frustrated that I couldn’t use |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
/run-acceptance-tests |
Please view the results of the PR Build + Acceptance Tests Run Here |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
/run-acceptance-tests |
Please view the results of the PR Build + Acceptance Tests Run Here |
1 similar comment
Please view the results of the PR Build + Acceptance Tests Run Here |
LGTM. Thanks, @swen128! And many apologies for the delay getting this merged! I've included a test which confirms being able to set a new |
@justinvp |
Description
Fixes #4829, #2673
Checklist
I haven't added tests yet because I'm not sure which test directory is relevant with this PR (maybe
/tests/integration
?).