Sitelet https://web.archive.org/web/20200925145417/https://github.com/microsoft/TypeScript/pull/40275
Skip to content

/ TypeScript

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

Checkjs implies allowjs #40275

Merged
merged 5 commits into from Sep 1, 2020
Merged

Checkjs implies allowjs #40275

merged 5 commits into from Sep 1, 2020
+12 −10

Conversation

@sandersn
Copy link
Member

sandersn commented Aug 26, 2020

If it's not explicitly provided.

Note that it's still an error to explicitly provide checkJs: true, allowJs: false.

This change makes the compiler options easier to use and to describe.
sandersn added 4 commits Aug 26, 2020
@sandersn
Passing --checkJs always sets --allowJS
3f51a27 
Even if you have `"allowJs": false`. This is not a useful combination.

Changing this makes the compiler more friendly and easier to describe.
@sandersn
only set allowjs if not explicitly set
1f8f57c
@sandersn
remove stray newline
45a1437
@sandersn
restore bad config error
Loading status checks…
1ff3320
@sandersn
Copy link
Member Author

sandersn commented Aug 26, 2020

@DanielRosenwasser @RyanCavanaugh @orta Do you think the new behaviour is reasonable?

@sheetalkamat @weswigham Is createProgram the right place to put this?
@weswigham
Copy link
Member

weswigham commented Aug 26, 2020

For other defaulted flags dependent on other flags, we have something like a getStrictCompilerOptionValue helper that we use everywhere rather than checking the compiler options directly (no, I don't think createProgram handles all inputs - transpileModule is notable for probably being unaffected). You should probably add a getAllowJSCompilerOption helper that we use everywhere rather than checking the flags directly.
@sandersn
use an accessor function instead
Loading status checks…
b9943fd
@sandersn
Copy link
Member Author

sandersn commented Aug 26, 2020

I thought that might be the case. I switched to an accessor function getAllowJSCompilerOption instead.
@weswigham
weswigham approved these changes Aug 26, 2020
View changes
@sheetalkamat
sheetalkamat reviewed Aug 27, 2020
View changes
src/compiler/program.ts
@@ -3160,7 +3160,7 @@ namespace ts {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_when_option_target_is_ES3, "useDefineForClassFields");
}

if (options.checkJs && !options.allowJs) {
if (options.checkJs && !getAllowJSCompilerOption(options)) {

This comment has been minimized.

Sign in to view
@sheetalkamat

sheetalkamat Aug 27, 2020 Member

This seems incorrect condition. You want to check if options.allowJs === false here instead

This comment has been minimized.

Sign in to view
@sandersn

sandersn Sep 1, 2020 Author Member

Duh, thanks for the catch.

This comment has been minimized.

Sign in to view
@sandersn

sandersn Sep 1, 2020 Author Member

No, wait, that undoes the loosening from this PR. THe current code only errors when checkJs: true but allowJs: false explicitly.
@sheetalkamat
sheetalkamat approved these changes Aug 27, 2020
View changes
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 27, 2020

I'm not so sure how I feel about this. But I can't think of any reason why it's bad.
@sandersn sandersn added this to Not started in PR Backlog Sep 1, 2020
@sandersn
Copy link
Member Author

sandersn commented Sep 1, 2020

Chatted with @RyanCavanaugh, he couldn't either.

He also pointed out that maybe allowJs should default to true now. I'll investigate that next.
@sandersn sandersn merged commit b0011fe into master Sep 1, 2020
8 checks passed
8 checks passed
build (10.x)
Details
build (12.x)
Details
build (14.x)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
node10 Build #83455 succeeded
Details
node12 Build #83453 succeeded
Details
node14 Build #83454 succeeded
Details
PR Backlog automation moved this from Not started to Done Sep 1, 2020
@sandersn sandersn deleted the checkjs-implies-allowjs branch Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham

@sheetalkamat sheetalkamat

@orta orta

@DanielRosenwasser DanielRosenwasser

@RyanCavanaugh RyanCavanaugh

Assignees

@sandersn sandersn

Labels
Author: Team For Uncommitted Bug
Projects
PR Backlog
  
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@sandersn @weswigham @DanielRosenwasser @sheetalkamat @typescript-bot
You can’t perform that action at this time.