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

Allow decorators in JavaScript files #6881

Merged
merged 5 commits into from
Feb 16, 2016
Merged

Allow decorators in JavaScript files #6881

merged 5 commits into from
Feb 16, 2016

Conversation

billti
Copy link
Member

@billti billti commented Feb 3, 2016

Fixes #6872

@@ -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:
Copy link
Contributor

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.

Copy link
Member Author

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?).

Copy link
Contributor

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.

Copy link
Member

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.

@billti billti self-assigned this Feb 9, 2016
@billti
Copy link
Member Author

billti commented Feb 13, 2016

I've updated this to give the same warning in JavaScript that it gives in TypeScript, namely to set experimentalDecorators. Note I changed the error message slightly also, as a JavaScript user is unlikely to be using the command-line compiler, but will want to know they need to set the option (in the ?sconfig.json file).

I can port this to release-1.8 also if you think this is low risk enough. (Seems desirable for Salsa).

@@ -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.": {
Copy link
Member

Choose a reason for hiding this comment

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

"the 'experimentalDecorators' option"

@DanielRosenwasser
Copy link
Member

Looks good to me. Just need to get the tests working (see #7069).

@billti
Copy link
Member Author

billti commented Feb 13, 2016

@mhegazy Any thoughts? Else I'll port this to 1.8 also before Tuesday.

I do think ideally the error logic should probably be in the same location for TypeScript & JavaScript, (and probably not in the type checker as its really a syntax issue), but we could look at refactoring this as part of #6802.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 16, 2016

sorry for the delay. 👍

@RyanCavanaugh
Copy link
Member

👍

billti added a commit that referenced this pull request Feb 16, 2016
Allow decorators in JavaScript files
@billti billti merged commit 9cc092a into master Feb 16, 2016
@billti billti deleted the issue6872 branch February 16, 2016 19:20
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants