Raise recoverable error for type members with invalid modifiers#12785
Raise recoverable error for type members with invalid modifiers#12785nicolo-ribaudo merged 9 commits intobabel:mainfrom
Conversation
sosukesuzuki
commented
Feb 9, 2021
| 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:
|
nicolo-ribaudo
left a comment
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 |
| 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.
| 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) |