-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
Conversation
@typescript-bot pack this |
Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at d3b479e. You can monitor the build here. |
@DanielRosenwasser we probably need this patch for 4.3 ? |
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.
I think I get it, but please see my comment.
// 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. |
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.
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.
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.
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.
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.
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?
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.
correct.
Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
@typescript-bot pack this |
Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at cee9f40. You can monitor the build here. |
@typescript-bot cherry pick this to release-4.3 branch |
@typescript-bot cherry pick this to release-4.3 |
@sheetalkamat @DanielRosenwasser Looks like you need a dash in "cherry-pick" for the bot to activate |
@typescript-bot cherry-pick this to release-4.3 |
Heya @sheetalkamat, I've started to run the task to cherry-pick this into |
Hey @sheetalkamat, I've opened #44431 for you. |
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>
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>
Fixes #44305