-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Allow decorators in JavaScript files #6881
Conversation
@@ -1168,9 +1168,6 @@ namespace ts { | |||
let typeAssertionExpression = <TypeAssertion>node; | |||
diagnostics.push(createDiagnosticForNode(typeAssertionExpression.type, Diagnostics.type_assertion_expressions_can_only_be_used_in_a_ts_file)); | |||
return true; | |||
case SyntaxKind.Decorator: |
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.
since this is a non-standard feature, we should ask you to set a flag.. i.e. error if the flag is not set, and ask the user to specify the flag.
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.
Hmmmm. I'm conflicted on this one, as it seems maybe a little user hostile. (Kind of like we used to require a 'module' setting for you to write a module). Its not like they are going to write valid decorator syntax by accident, they opt in to decorators by virtue of writing one, and there is only one setting for the config option (true
), but we'd go make them add a config file and the option anyway, (even though for many Salsa users they are just editing code, not compiling).
On the other hand, I get that the feature is "experimental" and we have precedent. Let's chat tomorrow, and I'll try and prepare the change anyway in the meantime. ( @egamma Any strong thoughts?).
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 am just worried of breaking these users later with no warning. the switch is a implicit agreement that you may be broken. not sure if this changes your feelings when you really are broken though.
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.
My only strong feeling is that showing an error is too much, showing a warning that this is an experimental feature would be more appropriate. But then the warning has to tell me how to get rid of it.
I've updated this to give the same warning in JavaScript that it gives in TypeScript, namely to set I can port this to |
@@ -687,7 +687,7 @@ | |||
"category": "Error", | |||
"code": 1218 | |||
}, | |||
"Experimental support for decorators is a feature that is subject to change in a future release. Specify '--experimentalDecorators' to remove this warning.": { | |||
"Experimental support for decorators is a feature that is subject to change in a future release. Set 'experimentalDecorators' to remove this warning.": { |
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.
"the 'experimentalDecorators' option"
Looks good to me. Just need to get the tests working (see #7069). |
sorry for the delay. 👍 |
👍 |
Allow decorators in JavaScript files
Fixes #6872