babel
babel copied to clipboard
More consistent API when using errorRecovery
Q | A |
---|---|
Fixed Issues? | Fixes #14175 |
Patch: Bug Fix? | No |
Major: Breaking Change? | No |
Minor: New Feature? | Yes |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | No |
License | MIT |
As described in the original issue #14175, this PR intends to make all errors recoverable when using the errorRecovery
option, such that only "legitimate programming mistakes" on our part cause an error to (unintentionally) be thrown from parse
. This significantly simplifies usage of error recovery, by having one consistent place to look for errors: the errors
property of the returned object. This is in contrast to the way things currently work, where if you turn on errorRecovery
, you both have to check the errors
property "on success", as well as catching non-recoverable errors "on failure", which itself could be a "parse error" or of course a "logic error" on our part. Often this results in the user having to wrap parse
in their own wrapper function to consolidate these results (we for example do this in Babel's own tests). From a type perspective, you can think of this as changing the type of parse
from something like:
Either<ParserOutput, SyntaxError | Error>
to just:
Either<ParserOutput, Error>
In order to accomplish this across the board on all errors, the idea is to simply return an empty Program
for the AST in the cases that are currently "unrecoverable". The thinking here is that the AST isn't trustworthy when in an errored state anyways, and is at best a "best guess", so an empty Program
represents the "best guess" of a (currently) unrecoverable error. As we make more errors fully recoverable, their corresponding Program
s will thus also flesh out more. This is analogous to the current state of affairs when adding recoverability to existing unrecoverable errors, and is also the way these "wrapper" functions are usually implemented in userland.
Implementation
Although original goal was to make errorRecovery: true
behave in the manner described above, we discovered that some consumers rely on the particularities of the current behavior, so we decided it would make more sense to simply have a new possible value for errorRecovery
of "always"
, and propose that this should be the behavior for true
in Babel 8 (at which point we'd return to only allowing false
and true
). This allows us to, for example, use this behavior in the Babel tests (and we'd actually also use it at RunKit), but not cause any waves for anyone else that is using errorRecovery unless they specifically turn it on.
Going forward/proposal
I believe this new option value should be temporary and should probably just be removed in Babel 8 in favor of true
. However given the unknown number of existing projects which might break from this change, I thought it best that the change be made for a major version upgrade, not a minor update.
Specific implementation notes
- Sometimes an empty file/program is returned, but in the case of
parseExpression
there is no such thing as an "empty expression", so what's returned is just an object with errors and comments (because comments is a required field forParserOutput
s). - In a couple places the parser tries a few different passes before failing. In particular it will try to parse certain code as JSX and when it fails will go on to parse it differently. If that second parse pass fails the original JSX parse error gets thrown. In this case I've restored the parse state before the error gets thrown. This ensures that the parse state matches the control flow, and as a result the JSX error is already in the state.errors list.
- The test fixtures have been updated because most errors no longer throw (some unicode invalid character errors still throw though, but that's independent of the parser itself). So the fixtures have moved most of those errors from the options file to the output file.
- Babel 8 can sometimes recover better than Babel 7. That means Babel 8 sometimes has more errors in the error list than babel 7. Because of this I added a mechanism to the test suite so that we can specify a different output file (if running babel 8 and the outputs are indeed different).
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51719/
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51719/
This PR also gets us one step closer to being able to update our testing infrastructure to test errors both with errorRecovery true
("always"
in this case) and false
to check that they produce the same kind of errors (which would catch cases like this: https://github.com/babel/babel/issues/14146 ).