Skip to content
This repository was archived by the owner on Jan 14, 2019. It is now read-only.

BREAKING: throw when invalid modifiers are added to parameter properties #51

Closed
wants to merge 1 commit into from

Conversation

j-f1
Copy link
Contributor

@j-f1 j-f1 commented Dec 16, 2018

BREAKING CHANGE: static and exports modifiers are no longer permitted on parameter properties

These result in a compile-time error, as shown in the playground.

BREAKING CHANGE: `static` and `exports` modifiers are no longer permitted on parameter properties
@j-f1
Copy link
Contributor Author

j-f1 commented Dec 17, 2018

This would be covered by bradzacher/eslint-plugin-typescript#245 & eslint/typescript-eslint-parser#547 (comment). Feel free to close if those solutions are preferred.

@j-f1 j-f1 changed the title feat: throw when invalid modifiers are added to parameter properties BREAKING: throw when invalid modifiers are added to parameter properties Dec 17, 2018
@JamesHenry
Copy link
Owner

JamesHenry commented Dec 21, 2018

@j-f1 I'm curious about "no longer permitted" and then providing a playground showing type check errors.

This for me just confirms that it has always been the case in TypeScript that things like this are parseable and so therefore produce an AST. TypeScript then uses that to inform the separate checking step.

As a result other parsers in the ecosystem are a lot less forgiving than tsc and provide immediate feedback about syntactic and semantic issues.

That is not to say I am against moving away from the "loose" behaviour towards consistency with other parsers (particularly babel), I was just curious about you implying that something had changed recently about this behaviour...

As you mentioned a couple of things have been proposed, and I think maybe starting with just offering an option to throw on semantic errors we can retrieve from the TypeScript program would be a great start.

If that is not enough in some cases we can add in special handling as you have done here (although again, probably behind the new option).

I will put together a PR for that now, and then we can see if we want to press ahead with this one on top.

Sound good?

@j-f1
Copy link
Contributor Author

j-f1 commented Dec 21, 2018

Sounds good. Throwing the syntactic and semantic errors will probably make this PR irrelevant.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants