Skip TSAsExpression when transforming spread in CallExpression #11404

oliverdunk · 2020-04-10T23:26:23Z

Q	A
Fixed Issues?	Fixes #11400
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

In babel-plugin-transform-spread, when transforming spreaded arguments in a CallExpression, we weren't handling the case where the callee was a TSAsExpression. This meant we incorrectly determined the this of our apply call, transforming:

(dog.bark as any)(...args)

... into ...

dog.bark.apply(void 0, args)

I've fixed that by exploring deeper, into the expression of the TSAsExpression.

oliverdunk · 2020-04-10T23:34:34Z

Not sure if there's a better solution here. It feels like the ideal would be passing the entire path to a "get me this without the TypeScript" function.

nicolo-ribaudo · 2020-04-11T11:11:41Z

Other tests:

class A {
  #x;
  
  fn() {
    (this.#x as any)();
  }
}

(a.b as any)?.()

And also with Flow type casts ((a.b: any)(...args)) and old-style TS casts ((<any> a.b)(...args)).

Also, it would be nice to make it work when the createParenthesizedExpressions parser option (which creates explicit nodes for (foo)) is enabled with (a.b)(...args).

It feels like the ideal would be passing the entire path to a "get me this without the TypeScript" function.

I agree! Missing the correct this with type casts is a common pitfall and a shared function would be useful. What about a new @babel/helper-get-call-context package?

packages/babel-plugin-transform-spread/src/index.js

oliverdunk · 2020-05-03T22:39:07Z

Thanks for the feedback @nicolo-ribaudo! I've created the package, and added a bunch of extra tests - let me know what you think.

I wasn't able to test (a.b as any)?.(), because I don't think babel-plugin-transform-spread supports optional call expressions. Do you agree? If so, I can open an issue to fix that.

I have two quick tweaks which I will make tomorrow morning:

My new package should probably support optional call expressions (it nearly does, but I just realised that I made a mistake by asserting I was passed a call expression).
It looks like the tests are failing. I'll try merging with master, unless you can see that something else is going wrong.

oliverdunk · 2020-05-04T19:50:18Z

Thanks for the feedback @nicolo-ribaudo! I've created the package, and added a bunch of extra tests - let me know what you think.

I wasn't able to test (a.b as any)?.(), because I don't think babel-plugin-transform-spread supports optional call expressions. Do you agree? If so, I can open an issue to fix that.

I have two quick tweaks which I will make tomorrow morning:

My new package should probably support optional call expressions (it nearly does, but I just realised that I made a mistake by asserting I was passed a call expression).

It looks like the tests are failing. I'll try merging with master, unless you can see that something else is going wrong.

Made changes as promised.

codesandbox-ci · 2020-05-04T19:52:25Z

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 e6c3a4c:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

babel-bot · 2020-05-04T19:52:37Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/26343/

nicolo-ribaudo · 2020-05-08T21:28:25Z

@oliverdunk The tests I proposed in #11404 (comment) where without spread 😛

The caller is also needed by the optional chaining plugin.

nicolo-ribaudo · 2020-05-17T21:46:47Z

...babel-plugin-proposal-private-methods/test/fixtures/call-context/ts-as-class-field/output.js

+  }
+
+  fn() {
+    _classPrivateFieldGet(this, _x)();


This still call still doesn't have the correct this!

Thanks, admittedly I didn't look close enough and thought it was right! That's a tough one, because we're going in the opposite direction. The transformation is done by babel-helper-member-expressions-to-functions, which takes a member, and then checks to see if its parent is a call expression.

I was able to fix it with the following diff: https://hastebin.com/onotaqusiw.diff

It seems like this would be an issue for the entire helper. Maybe the helper should be a type stripper, instead of just something for calls? It would export a function for each direction.

The final tricky bit would be things like this:

if (parentPath.isAssignmentExpression({ left: node })) {

We could probably change that to this?

if (parentPath.isAssignmentExpression({ left: previousParent })) {

With previousParent set to node initially, and then the last parent each time we go a level up?

It crossed my mind that if we move the logic to a plugin, we can't keep track of the previous parent. Maybe this logic has to be duplicated in this case.

Maybe we could keep this for a follow-up PR then

Sounds good. I have a first draft, so I'll open a WIP MR with that once this is merged.

oliverdunk · 2020-05-17T22:41:09Z

I'm not sure if I missed something when adding my new package as a dependency. I thought Lerna would find it, but the tests are failing and make bootstrap errors too.

packages/babel-plugin-proposal-optional-chaining/package.json

packages/babel-helper-get-call-context/package.json

packages/babel-plugin-proposal-optional-chaining/src/index.js

oliverdunk · 2020-05-23T13:07:28Z

We should probably strip types from optional member expressions, too! 😅

Does replacing @babel/helper-get-call-context with @babel/helper-skip-types sound ok?

nicolo-ribaudo · 2020-05-23T20:06:03Z

We should probably strip types from optional member expressions, too! 😅

What do you mean?

Btw, for some other PRs (e.g. #11552) it might be useful to add a function to this package that, give the callee, returns the call (the opposite of the one introduced by this PR).

oliverdunk · 2020-05-23T20:10:28Z

What do you mean?

I'm thinking of this case: (a?.b as ExampleType)?.c

oliverdunk · 2020-06-29T22:41:00Z

Thanks for the feedback! Found some time to take a look at it.

@nicolo-ribaudo, I've hopefully addressed all of your comments. Would love for you to take another look.

@JLHwung, I need to fix the helper call where I wrongly pass an ASTNode. I'll ping you when that's done. In the meantime, I did leave a response to another bit of feedback you left.

Also changed the base branch to main, good call on the switch!

packages/babel-helper-skip-transparent-expr-wrappers/README.md

existentialism

Just a small nit, but otherwise LGTM, nice work @oliverdunk!

oliverdunk · 2020-07-03T18:05:36Z

Thanks for the reviews @existentialism and @kaicataldo! I went ahead and pushed the rename.

oliverdunk · 2020-07-03T18:08:52Z

The changelog comes from the PR title, right? If so, we might want something broader:

Introduce @babel/helper-skip-transparent-expression-wrappers package, to skip irrelevant types and parentheses in plugins

JLHwung

LGTM with some comments.

packages/babel-helper-skip-transparent-expression-wrappers/package.json

packages/babel-helper-skip-transparent-expression-wrappers/src/index.js

JLHwung · 2020-07-03T19:11:33Z

packages/babel-helper-skip-transparent-expression-wrappers/src/index.js

+import * as t from "@babel/types";
+import type { NodePath } from "@babel/traverse";
+
+export function getCallContext(callPath: NodePath): NodePath {


I think getCallContext is a consumer of helper-skip-transparent-expression-wrappers and it should not be in this package.

Do we have a test case on Expected type "CallExpression" or "OptionalCallExpression" ? I think we can go with skipTransparentExprWrappers(path.get("callee")) and not worry about path.type, they should be validated by transformers or implied by how the path is filtered. i.e. "CallExpression|OptionalCallExpression"(path).

I'm alright with that. I think it made more sense when the scope of the package was smaller.

oliverdunk · 2020-07-03T19:39:15Z

@JLHwung All sorted 😄

packages/babel-plugin-transform-spread/test/fixtures/call-context/flow-type-cast/options.json

oliverdunk · 2020-07-22T21:55:39Z

@JLHwung Removed the inline helper functions! This should be good for another look.

I'm happy to keep tweaking this, but at the same time, I won't be offended if you want to jump in with a few commits. If you still have some tidy up in mind it might be quicker :)

JLHwung

Awesome!

oliverdunk · 2020-07-30T19:16:52Z

@JLHwung Thanks for the merge!

JLHwung · 2020-07-30T19:23:41Z

@oliverdunk Thank you for all the efforts!

oliverdunk added 2 commits April 11, 2020 00:18

Skip TSAsExpression when transforming spread in CallExpression

087405e

Merge branch 'master' into od/11400-typescript-spread-as

dc5316e

nicolo-ribaudo added area: flow area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 11, 2020

nicolo-ribaudo reviewed Apr 11, 2020

View reviewed changes

packages/babel-plugin-transform-spread/src/index.js Outdated Show resolved Hide resolved

Create @babel/helper-get-call-context package

e090927

oliverdunk force-pushed the od/11400-typescript-spread-as branch from 44027e1 to e090927 Compare May 3, 2020 22:32

oliverdunk requested a review from nicolo-ribaudo May 3, 2020 22:39

oliverdunk added 2 commits May 4, 2020 20:47

Support OptionalCallExpressions

b41cc10

Merge branch 'master' into od/11400-typescript-spread-as

f406e41

oliverdunk added 2 commits May 16, 2020 21:06

Merge branch 'master' into od/11400-typescript-spread-as

a0bb643

Use helper in optional chaining plugin, and move tests

2bdb62d

nicolo-ribaudo reviewed May 17, 2020

View reviewed changes

nicolo-ribaudo reviewed May 18, 2020

View reviewed changes

packages/babel-plugin-proposal-optional-chaining/package.json Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed May 22, 2020

View reviewed changes

packages/babel-helper-get-call-context/package.json Outdated Show resolved Hide resolved

packages/babel-plugin-proposal-optional-chaining/src/index.js Outdated Show resolved Hide resolved

oliverdunk added 3 commits May 23, 2020 13:01

Update package.json files

06d092f

Use dot notation to access property

ed10970

Remove private method tests until future MR

1d957b3

oliverdunk requested a review from nicolo-ribaudo June 29, 2020 22:36

oliverdunk added 2 commits June 30, 2020 22:06

Merge branch 'main' into od/11400-typescript-spread-as

24d550a

Pass correct parameter type to skipTransparentExprWrappers

7069abe

oliverdunk requested a review from JLHwung June 30, 2020 21:28

existentialism reviewed Jul 3, 2020

View reviewed changes

packages/babel-helper-skip-transparent-expr-wrappers/README.md Outdated Show resolved Hide resolved

existentialism approved these changes Jul 3, 2020

View reviewed changes

kaicataldo approved these changes Jul 3, 2020

View reviewed changes

Rename to babel-helper-skip-transparent-expression-wrappers

0704742

JLHwung reviewed Jul 3, 2020

View reviewed changes

Remove getCallContext helper

f6cc6e2

Fixed exports key

cea347f

JLHwung reviewed Jul 3, 2020

View reviewed changes

packages/babel-plugin-transform-spread/test/fixtures/call-context/flow-type-cast/options.json Outdated Show resolved Hide resolved

Preserve types in babel-plugin-transform-spread tests

d654efc

oliverdunk requested a review from JLHwung July 3, 2020 20:56

JLHwung reviewed Jul 3, 2020

View reviewed changes

packages/babel-plugin-transform-spread/test/fixtures/call-context/flow-type-cast/options.json Outdated Show resolved Hide resolved

Use external-helpers to avoid inlining helper functions in tests

e6c3a4c

oliverdunk requested a review from JLHwung July 22, 2020 21:55

JLHwung approved these changes Jul 23, 2020

View reviewed changes

JLHwung merged commit db56261 into babel:main Jul 30, 2020

oliverdunk deleted the od/11400-typescript-spread-as branch July 30, 2020 19:10

bz2 mentioned this pull request Sep 29, 2020

Unexpected token with webpack after upgrade to 2.0.0 preactjs/prefresh#192

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 30, 2020

github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip TSAsExpression when transforming spread in CallExpression #11404

Skip TSAsExpression when transforming spread in CallExpression #11404

oliverdunk commented Apr 10, 2020 •

edited by gitpod-io bot

oliverdunk commented Apr 10, 2020

nicolo-ribaudo commented Apr 11, 2020 •

edited

oliverdunk commented May 3, 2020

oliverdunk commented May 4, 2020

codesandbox-ci bot commented May 4, 2020 •

edited

babel-bot commented May 4, 2020 •

edited

nicolo-ribaudo commented May 8, 2020

nicolo-ribaudo May 17, 2020

oliverdunk May 17, 2020

oliverdunk May 18, 2020

nicolo-ribaudo May 22, 2020

oliverdunk May 23, 2020

oliverdunk commented May 17, 2020 •

edited

oliverdunk commented May 23, 2020

nicolo-ribaudo commented May 23, 2020

oliverdunk commented May 23, 2020

oliverdunk commented Jun 29, 2020

existentialism left a comment •

edited

oliverdunk commented Jul 3, 2020

oliverdunk commented Jul 3, 2020

JLHwung left a comment

JLHwung Jul 3, 2020

oliverdunk Jul 3, 2020

oliverdunk commented Jul 3, 2020

oliverdunk commented Jul 22, 2020

JLHwung left a comment

oliverdunk commented Jul 30, 2020

JLHwung commented Jul 30, 2020

Navigation Menu

Skip TSAsExpression when transforming spread in CallExpression #11404

Skip TSAsExpression when transforming spread in CallExpression #11404

Conversation

oliverdunk commented Apr 10, 2020 • edited by gitpod-io bot

oliverdunk commented Apr 10, 2020

nicolo-ribaudo commented Apr 11, 2020 • edited

oliverdunk commented May 3, 2020

oliverdunk commented May 4, 2020

codesandbox-ci bot commented May 4, 2020 • edited

babel-bot commented May 4, 2020 • edited

nicolo-ribaudo commented May 8, 2020

nicolo-ribaudo May 17, 2020

Choose a reason for hiding this comment

oliverdunk May 17, 2020

Choose a reason for hiding this comment

oliverdunk May 18, 2020

Choose a reason for hiding this comment

nicolo-ribaudo May 22, 2020

Choose a reason for hiding this comment

oliverdunk May 23, 2020

Choose a reason for hiding this comment

oliverdunk commented May 17, 2020 • edited

oliverdunk commented May 23, 2020

nicolo-ribaudo commented May 23, 2020

oliverdunk commented May 23, 2020

oliverdunk commented Jun 29, 2020

existentialism left a comment • edited

Choose a reason for hiding this comment

oliverdunk commented Jul 3, 2020

oliverdunk commented Jul 3, 2020

JLHwung left a comment

Choose a reason for hiding this comment

JLHwung Jul 3, 2020

Choose a reason for hiding this comment

oliverdunk Jul 3, 2020

Choose a reason for hiding this comment

oliverdunk commented Jul 3, 2020

oliverdunk commented Jul 22, 2020

JLHwung left a comment

Choose a reason for hiding this comment

oliverdunk commented Jul 30, 2020

JLHwung commented Jul 30, 2020

oliverdunk commented Apr 10, 2020 •

edited by gitpod-io bot

nicolo-ribaudo commented Apr 11, 2020 •

edited

codesandbox-ci bot commented May 4, 2020 •

edited

babel-bot commented May 4, 2020 •

edited

oliverdunk commented May 17, 2020 •

edited

existentialism left a comment •

edited