Do not parse template arguments in JavaScript files.#36673
Do not parse template arguments in JavaScript files.#36673andrewbranch merged 1 commit intomicrosoft:masterfrom
Conversation
Worse, they may get no error message if they accidentally include a single |
|
@fatcerberus ack, but that's fundamentally because in JS, |
|
Tentatively filing this as a breaking change even though that’s sort of debatable. Most of the time this is going to be swapping one error for another; very occasionally it will be removing an undesired error (which is the point of the PR, #36662), but as @fatcerberus pointed out, there could be a possible scenario where you were getting a desired error (type arguments can’t be used in JS) before, and aren’t now. I’d like to dig into the specifics of whether that could actually ever happen without getting a new error in the vein of “Operator '<' cannot be applied to types...” I guess if enough of the variables in play are declared as |
Also FWIW, I do think this would be an easy win if it’s not ambiguous. |
|
For |
But not when a bunch of stuff is |
|
Well shame on you for having |
|
Jokes aside, I think I'd be surprised to see a user write explicit type arguments in a JS file that's unchecked. TypeScript users already see explicit type arguments as a bit of a code smell. |
|
One possible exception is .js files that are flow-checked. I'm not sure whether there are more of those than of .js files with syntax that is currently parsing as type arguments. |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f308ac1. You can monitor the build here. |
|
Just want to make sure we still give good errors on
Maybe @bterlson can help find examples that are valid JS we also currently have issues with |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at f308ac1. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f308ac1. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at f308ac1. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at f308ac1. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at f308ac1. You can monitor the build here. |
|
@DanielRosenwasser Here they are:Comparison Report - master..36673
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
Thanks for your patience, everyone. I think we should take this 🌟
|
Marking this with the |
6 participants
Fixes #36662.
This is somewhat preliminary. The main drawback of the change is that users will get substantially worse error messages if they accidentally include
<Type, Arguments>in JavaScript files (see the associated errors.txt changes).To mitigate, we could:
<Element<Type, Arg> x=1>). I don't think this is ambiguous with any JS syntax.fnCall<Type, Argument>()(2) could be tricky to get right enough to really improve developer's life, so I think whether it's worth doing depends on how likely we think the scenario is in the first place.