Disable ES transforms (#602) (#622)

[webschik]

The idea of this PR is to allow to opt-out the transformations of "modern" ES features like optional chaining, numbers separators, etc.

Please check issues #602, #622 for more details.

I've tried to implement such a flag, as @alangpierce suggested in #602.
The implementation appeared bigger than I expected as I had to implement changes in 3 main modules:

1. RootTransformer – omit unnecessary ES transformers
2. HelperManager – disable unused helpers emitting (reverted after finding in Disable ES transforms (#602) (#622) #623 (comment))
3. TokenProcessor – disable helpers insertion into a final code, e.g. _optionalChain(...) wrapper.

All described scenarios were covered by test cases.

I can't say that this fix is the ideal solution, so please let me know you think we need to fix this in another way.

P.S. Small comment from contribution experience – have you considered migrating to Yarn2? :)

[codecov]

Codecov Report

Merging #623 (aeac203) into main (7784361) will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
+ Coverage   82.16%   82.18%   +0.01%     
==========================================
  Files          56       56              
  Lines        5870     5876       +6     
  Branches     1324     1327       +3     
==========================================
+ Hits         4823     4829       +6     
  Misses        763      763              
  Partials      284      284              

Impacted Files	Coverage Δ
src/Options-gen-types.ts	100.00% <ø> (ø)
src/Options.ts	100.00% <ø> (ø)
src/TokenProcessor.ts	92.10% <66.66%> (-1.54%)	⬇️
src/index.ts	90.19% <100.00%> (+0.19%)	⬆️
src/transformers/RootTransformer.ts	93.62% <100.00%> (+1.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7784361...aeac203. Read the comment docs.

[github-actions]

Benchmark results

Before this PR: 324.7 thousand lines per second
After this PR: 325.4 thousand lines per second

Measured change: 0.21% faster (0.77% slower to 1.35% faster)
Summary: Likely no significant difference

[webschik]

Oops, I missed test-with-coverage command... Will add more tests to align the code coverage

[alangpierce]

Thanks! I'm on vacation without my computer for the next 5 days or so, but can take a closer look when I get back.

You're right that it's a bit more involved than I was thinking, thanks for working through the details. A few initial thoughts:

• I think the helper code changes shouldn't be necessary. It will only emit helpers that are actually used, so as long as the TokenProcessor doesn't use it, the helpers won't be emitted. I may be missing a detail, though.
• Ideally this would also skip the class transform (in getClassInfo and RootTransformer), but still transform TS constructor param initializers. It could hopefully be done by passing the flag to those code paths and doing a lot less when transforms are disabled.

[webschik]

@alangpierce you're right. I've reverted all changes in HelperManager. Actually, I started fixing this class first and only then found a problem with TokenProcessor. Probably missed the relationship between them... 😊
P.S. Enjoy your vacation 👍

[alangpierce]

Thank you! I'll merge this PR, but with a few follow-ups before releasing:

• The extra await is buggy, I believe. I can write a quick test for that and remove it in a follow-up PR.
• The class transform should be disabled as well. Some of the details were discussed back in Add disableLegacyClassFieldSupport option #304 , the main challenge is that we still want to transform TypeScript constructor initializers. Hopefully it'll be reasonable to implement, but it does require some thinking about how to best modify getClassInfo and related code.
• I can update the README to mention the new flag.

have you considered migrating to Yarn2?

Thanks for the suggestion, it has been on my radar, but I haven't prioritized it yet.

[alangpierce]

I think this await code should actually be skipped altogether here.

It's for cases like a?.b(await f()). Normally, the inner code is wrapped in an arrow function, but if it uses await, then it needs to instead use an async arrow function and be wrapped in a different wrapper function _asyncOptionalChain and the whole thing needs to have an await at the front. If we're using native ?., then the extra await isn't necessary (and could potentially cause problems).

[webschik]

Sorry, I completely forgot to update README...
Regarding await it seems you're right, I can't recall what use case I had for this.resultCode += "await ".
But as all tests pass, so probably it's safe to go without it.

Thanks for your help, I will close #622 !