Raise recoverable error for type members with invalid modifiers #12785
Conversation
sosukesuzuki
commented
Feb 9, 2021
sosukesuzuki
commented
| Q | A |
|---|---|
| Fixed Issues? | Fixes #12767 |
| Patch: Bug Fix? | Y |
| Tests Added + Pass? | Yes |
| License | MIT |
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/40440/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b79fd6b:
|
sosukesuzuki
added
area: typescript
pkg: parser
PR: Bug Fix 🐛
There was a problem hiding this comment.
Is there a specific reason for ededec2? I actually preferred the previous version.
| [ | ||
| "readonly", | ||
| "declare", | ||
| "abstract", | ||
| "private", | ||
| "protected", | ||
| "public", | ||
| "static", | ||
| ], |
There was a problem hiding this comment.
| [ | |
| "readonly", | |
| "declare", | |
| "abstract", | |
| "private", | |
| "protected", | |
| "public", | |
| "static", | |
| ], | |
| ["readonly", ...denyModifiers], |
so that the two lists don't get out of sync.
This is because |
| @@ -207,6 +210,7 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
| accessibility?: N.Accessibility, | |||
| }, | |||
| allowedModifiers: TsModifier[], | |||
| callback?: (startPos: number, modifer: TsModifier) => void, | |||
There was a problem hiding this comment.
Alternative interface design
tsParseModifiers(
modified: {
[key: TsModifier]: ?true,
accessibility?: N.Accessibility,
},
allowedModifiers: TsModifier[],
disallowedModifiers: TsModifier[],
errorTemplate: string
): voidSo we can raise errors when a disallowed modifier is seen and it can be reused in other situations. Introducing callback to parsers can be very nasty.
| disallowedModifiers && | ||
| disallowedModifiers.includes(modifier) | ||
| ) { | ||
| this.raise(startPos, errorTemplate, modifier); |
There was a problem hiding this comment.
nit: we can assume errorTemplate is non null if disallowedModifiers is provided. Overall this method not a public interface so we don't have to be defensive.
| @@ -207,6 +210,8 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
| accessibility?: N.Accessibility, | |||
| }, | |||
| allowedModifiers: TsModifier[], | |||
| disallowedModifiers?: TsModifier[], | |||
| errorTemplate?: string, | |||
| ): void { | |||
| for (;;) { | |||
| const startPos = this.state.start; | |||
There was a problem hiding this comment.
We can pass down allowedModifiers.concat(disallowedModifier) to tsParserModifier so we don't have to duplicate disallowedModifier in the tsParseModifiers call.
There was a problem hiding this comment.
Nit: allowedModifiers.concat(disallowedModifier ?? [])
| if ( | ||
| errorTemplate && | ||
| disallowedModifiers && | ||
| disallowedModifiers.includes(modifier) |
There was a problem hiding this comment.
| disallowedModifiers.includes(modifier) | |
| disallowedModifiers?.includes(modifier) |
github-actions
bot
added
the
outdated
