-
Notifications
You must be signed in to change notification settings - Fork 143
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
Implement optional chaining and nullish coalescing #461
Comments
@a-x- sure, I don't have any problem with anyone forking the project and adding any features they want to their own fork. In this case, I wouldn't accept this transformation as a PR because it's not correct in all cases, and more generally I think a correct optional chaining implementation is likely difficult enough that I'd prefer to call it out of scope for Sucrase. But if there's an implementation that's correct and isn't too complicated, I'd be happy to consider it. Babel compiles it as:
which also requires finding a good place to declare Some issues with the
|
Thank you!
I think it should return null or undefined according to |
Hmm, the README from https://github.com/tc39/proposal-optional-chaining says it would be
But I know it has been a topic of discussion and they've gone back and forth on the exact behavior there. |
This is how Babel plugin handles optional chaining: https://github.com/babel/babel/blob/master/packages/babel-plugin-proposal-optional-chaining/src/index.js |
Optional Chaining is now Stage 3, and currently my hope is that Chrome/Node will get optional chaining soon enough that it's ok to skip in Sucrase. It's being tracked here and looks like it just got an initial implementation: https://www.chromestatus.com/feature/5748330720133120 It definitely seems like the most significant recent JS feature, so it might be high priority enough to go into Sucrase even if the implementation is really complex, but for now I'd prefer to wait and see, so closing this out for now. |
Figured it's worth bumping this now that typescript has released optional chaining (and nullish coalescing, but that isn't in any browsers yet) in 3.7. Curious to know what might push this one over the edge. For me keeping parity with stable typescript features would be ideal since I'm running both over the same code, but I don't know how common that use case is. Thanks for the hard work! Also, what would be the first step implementing, just to pull in the way babel does it and tweak from there? |
Yep, agreed! I've been thinking about this more over the last few days since the TS release, and I'll reopen this issue and repurpose it to built-in support in Sucrase. I'll add some more detail when I get a chance. Chrome Canary now has optional chaining available by default, and in Chrome Beta it's available behind the "Experimental JavaScript" flag, so it's already reasonable to use it in development with Sucrase (which will emit it unchanged) for running code in the browser. My team runs tests in Node, though, and it looks like Node 14 (to be released next year) will be the first version with support. We also lag behind the latest Node, so it will be a while before we have native support for the syntax in tests. It seems like a high-enough value syntax that it can be an exception to the "Sucrase is hesitant to implement upcoming JS features" rule. Sucrase transforms require a lot more thought and care than Babel transforms since Sucrase makes the parser as minimal as possible and transforms code in a left-to-right scan, so it's tricky, but I think there will be a way to make it work. |
Ooh, looks like optional chaining is actually available in node 13 behind a flag and there's a backport that will make it available in node 12 behind a flag: nodejs/node#30109 . Still seems useful to have in Sucrase, but hopefully runtime support will come relatively soon. |
I have tested Optional Chaining and Nullish Coalescing in Node 13 using the --harmony-optional-chaining and --harmony-nullish flags respectively. No issues with either. However, I tested out this function from https://node.green validation for Nullish Coalescing with Node 13 and it returns false:
Putting parentheses around the last three tests makes it work correctly:
I did not look at the Nullish Coalescing spec to see how the implementation differs in terms of parentheses requirements from when this test had been created. |
I worked through the details and different options by writing up a technical plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan . I'll get started on the implementation soon. |
Progress toward #461 Tech plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan Some details: * In the parser, operator parsing now keeps track of the start token index so that when we observe a `??` operator, we can mark the first token as being the start of an optional chain. * The token format now has fields for the number of optional chain starts and ends for each token, which the transformer will use to inject code. * Optional chaining code needs to use the HelperManager system, so I refactored it to be on the RootTransformer instead of just on one of them. * The actual start/end code injection is done by the token system so that it works alongside all existing transforms. * Since TokenProcessor now needs a HelperManager, I had to break the dependency cycle by making NameManager no longer depend on TokenProcessor, since it was only barely using it anyway. Some quick perf benchmarks show this as having potentially a 4% slowdown even for code not using nullish coalescing, though I wasn't able to track down any specific area of code responsible, and it may have been partially due to variability or other factors. Except for more egregious cases, I'll leave performance work as a follow-up.
Progress toward #461 Tech plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan Some details: * In the parser, operator parsing now keeps track of the start token index so that when we observe a `??` operator, we can mark the first token as being the start of an optional chain. * The token format now has fields for the number of optional chain starts and ends for each token, which the transformer will use to inject code. * Optional chaining code needs to use the HelperManager system, so I refactored it to be on the RootTransformer instead of just on one of them. * The actual start/end code injection is done by the token system so that it works alongside all existing transforms. * Since TokenProcessor now needs a HelperManager, I had to break the dependency cycle by making NameManager no longer depend on TokenProcessor, since it was only barely using it anyway. Some quick perf benchmarks show this as having potentially a 4% slowdown even for code not using nullish coalescing, though I wasn't able to track down any specific area of code responsible, and it may have been partially due to variability or other factors. Except for more egregious cases, I'll leave performance work as a follow-up.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan The parser now associates each access operation with the start token for the chain, and marks each chain containing an optional chain operation so that we can wrap it in a function call. This does not yet handle optional deletion, and there are a number of follow-up tests that would be good to write, but the feature seems to be working with this implementation.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan This ended up being a relatively small tweak to the optional chaining implementation: * There's a new helper that wraps the optionalChain helper, and I needed to add a way for helpers to depend on each other. * In most cases, we condition on the delete and non-delete cases based on whether there's a delete token just before the start of the optional chain. * To determine whether a subscript is the last of its chain (and therefore needs a delete operation), we can walk the tokens forward tracking depth until we get to either another subscript or the end of the chain. This means there was no need for any changes to the parser. Since it only walks between two adjacent subscripts, it takes linear extra time unless there's nesting, and only ever does the more expensive operation when processing an optional delete.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan This ended up being a relatively small tweak to the optional chaining implementation: * There's a new helper that wraps the optionalChain helper, and I needed to add a way for helpers to depend on each other. * In most cases, we condition on the delete and non-delete cases based on whether there's a delete token just before the start of the optional chain. * To determine whether a subscript is the last of its chain (and therefore needs a delete operation), we can walk the tokens forward tracking depth until we get to either another subscript or the end of the chain. This means there was no need for any changes to the parser. Since it only walks between two adjacent subscripts, it takes linear extra time unless there's nesting, and only ever does the more expensive operation when processing an optional delete.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan This ended up being a relatively small tweak to the optional chaining implementation: * There's a new helper that wraps the optionalChain helper, and I needed to add a way for helpers to depend on each other. * In most cases, we condition on the delete and non-delete cases based on whether there's a delete token just before the start of the optional chain. * To determine whether a subscript is the last of its chain (and therefore needs a delete operation), we can walk the tokens forward tracking depth until we get to either another subscript or the end of the chain. This means there was no need for any changes to the parser. Since it only walks between two adjacent subscripts, it takes linear extra time unless there's nesting, and only ever does the more expensive operation when processing an optional delete.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan This ended up being a relatively small tweak to the optional chaining implementation: * There's a new helper that wraps the optionalChain helper, and I needed to add a way for helpers to depend on each other. * In most cases, we condition on the delete and non-delete cases based on whether there's a delete token just before the start of the optional chain. * To determine whether a subscript is the last of its chain (and therefore needs a delete operation), we can walk the tokens forward tracking depth until we get to either another subscript or the end of the chain. This means there was no need for any changes to the parser. Since it only walks between two adjacent subscripts, it takes linear extra time unless there's nesting, and only ever does the more expensive operation when processing an optional delete.
Progress toward #461 This doesn't update it to latest, but the current revision had a number of usages that can be handled by Sucrase now.
Progress toward #461 This doesn't update it to latest, but the current revision had a number of usages that can be handled by Sucrase now.
Progress toward #461 For now, we only run two subdirectories and ignore various tests (sloppy mode, tests expecting syntax error, and TCO tests). This reveals a few issues with the current implementation, which will be addressed later. Since it's not a passing build yet, I'll leave it out of CI for now. Also drop the Travis build for Node 8 since one of the new dependencies requires Node 10. I won't officially drop Node 8 support just yet, but will soon, and it's at end-of-life now.
Progress toward #461 The previous implemented worked for normal JS syntax, but `f?.<T>()` needs a special case when working with tokens. Also, regular type argument syntax wasn't marking the open-paren correctly, so this fixes that as well.
Progress toward #461 The previous implemented worked for normal JS syntax, but `f?.<T>()` needs a special case when working with tokens. Also, regular type argument syntax wasn't marking the open-paren correctly, so this fixes that as well.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan We now scan for an await keyword to indicate that the nullish coalescing or optional chaining operation is an async one, and in that case, call a different helper and emit async arrow functions instead of regular ones. This needed a few extra pieces of information in the token structure: * We now keep track of the scope depth to distinguish awaits inside inner async functions. * We use a token field to mark that the start token is an async operation. * We now track the start token for each nullish coalescing operation so that we can reference it to determine whether to emit an async arrow function. The extra token state and the duplicated helpers are all a bit ugly, but hopefully there won't be much more complexity beyond this, and I may investigate ways to store the token state in a more concise way. And, of course, this transform will go away eventually when optional chaining is available in node.
…497) Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan We now scan for an await keyword to indicate that the nullish coalescing or optional chaining operation is an async one, and in that case, call a different helper and emit async arrow functions instead of regular ones. This needed a few extra pieces of information in the token structure: * We now keep track of the scope depth to distinguish awaits inside inner async functions. * We use a token field to mark that the start token is an async operation. * We now track the start token for each nullish coalescing operation so that we can reference it to determine whether to emit an async arrow function. The extra token state and the duplicated helpers are all a bit ugly, but hopefully there won't be much more complexity beyond this, and I may investigate ways to store the token state in a more concise way. And, of course, this transform will go away eventually when optional chaining is available in node.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan To allow super, we mostly just need to skip the code transform on the first subscript. We also need to add a `.bind(this)` when transforming the second subscript, which can be determined through a relatively straightforward token scan.
Progress toward #461 Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan To allow super, we mostly just need to skip the code transform on the first subscript. We also need to add a `.bind(this)` when transforming the second subscript, which can be determined through a relatively straightforward token scan.
I just published optional chaining and nullish coalescing support as 3.12.0, so closing this! |
@alangpierce just want to thank you for knocking this out so quick! |
a?.b
→a && a.b
Ok or fork?
The text was updated successfully, but these errors were encountered: