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

Handle strict flag when writing tsbuildinfo #44394

Merged
merged 3 commits into from Jun 3, 2021
Merged

Handle strict flag when writing tsbuildinfo #44394

merged 3 commits into from Jun 3, 2021

Conversation

sheetalkamat
Copy link
Member

Fixes #44305

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 2, 2021
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2021

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at d3b479e. You can monitor the build here.

@sheetalkamat
Copy link
Member Author

@DanielRosenwasser we probably need this patch for 4.3 ?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it, but please see my comment.

src/compiler/builder.ts Outdated Show resolved Hide resolved
Comment on lines +567 to +568
// Though this affects semantic diagnostics, affectsSemanticDiagnostics is not set here
// The value of each strictFlag depends on own strictFlag value or this and never accessed directly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of wonder whether it's just worth it to say this always has affectsSemanticDiagnostics set to true since it's unlikely that you'd toggle this but have no difference in effective strict options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better that we dont update unnecessarily so prefer to keep it like this. But not stuck on it so open to changing that if you feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the actual consequence of doing it like this? Like if someone had every individual strict flag enabled, and then toggled strict itself, we would be able to correctly identify that it had no effect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct.

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2021

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at cee9f40. You can monitor the build here.

@sheetalkamat sheetalkamat merged commit 9df7ecb into main Jun 3, 2021
@sheetalkamat sheetalkamat deleted the strictFlag branch June 3, 2021 23:15
@sheetalkamat
Copy link
Member Author

@typescript-bot cherry pick this to release-4.3 branch

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry pick this to release-4.3

@DavidZidar
Copy link

@sheetalkamat @DanielRosenwasser Looks like you need a dash in "cherry-pick" for the bot to activate

@sheetalkamat
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.3

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 4, 2021

Heya @sheetalkamat, I've started to run the task to cherry-pick this into release-4.3 on this PR at cee9f40. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @sheetalkamat, I've opened #44431 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Jun 4, 2021
Component commits:
b6754e4 Add test showing how setting strict is not preserved in tsbuildinfo Test for microsoft#44305

d3b479e Handle strict flag when writing tsbuildinfo Fixes microsoft#44305

cee9f40 Apply suggestions from code review
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
sheetalkamat added a commit that referenced this pull request Jun 4, 2021
Component commits:
b6754e4 Add test showing how setting strict is not preserved in tsbuildinfo Test for #44305

d3b479e Handle strict flag when writing tsbuildinfo Fixes #44305

cee9f40 Apply suggestions from code review
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental builds with .tsbuildinfo-files are not faster than non incremental builds
6 participants