Skip to content
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

Fix compiling async arrows in uncompiled class fields #14752

Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 13, 2022

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

This PR should be reviewed commit-by-commit: the first one is a refactor to help TS inference understand the types of different variables (we had three different logic paths with different types all mixed together); the second one is the bug fix.

The generated output for async functions in uncompiled class fields is ugly, but it's uncommon enough that it should be fine (but not too uncommon, because Angular forces async functions compilation).

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Async Functions labels Jul 13, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jul 13, 2022

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

@nicolo-ribaudo nicolo-ribaudo changed the title Refactor helper-wrap-function for better type inference Fix compiling async arrows in uncompiled class fields Jul 13, 2022
let node;
if (path.isArrowFunctionExpression()) {
path = path.arrowFunctionToExpression({ noNewArrows });
node = path.node as t.FunctionDeclaration | t.FunctionExpression;
Copy link
Contributor

@JLHwung JLHwung Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't TS already infer path.node types here?

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it still includes t.ArrowFunctionExpression 😕

Copy link
Contributor

@JLHwung JLHwung Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it is likely confused by the reassignment.

@@ -151,7 +158,7 @@ export function arrowFunctionToExpression(
specCompliant?: boolean | void;
noNewArrows?: boolean;
} = {},
) {
): NodePath<Exclude<t.Function, t.Method | t.ArrowFunctionExpression>> {
Copy link
Contributor

@JLHwung JLHwung Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.arrowFunctionToExpression is an API, should we defer the changes to minor?

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have modified these internal plugin APIs in the past even in patch releases, and this change isn't adding "new functionality" but just exposing more info from an existing functionality.

However, if you prefer to wait that's fine

Copy link
Contributor

@JLHwung JLHwung left a comment

The return value of arrowFunctionToExpression is practical. If someone reported that their workflow is broken by this PR, we can reland it in the minor.

@nicolo-ribaudo nicolo-ribaudo merged commit 4f278dc into babel:main Jul 18, 2022
38 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the async-in-uncompiled-class-field branch Jul 18, 2022
isRestElement,
returnStatement,
} from "@babel/types";
import type * as t from "@babel/types";

type ExpressionWrapperBuilder<ExtraBody extends t.Node[]> = (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never work on a project where we needed such a custom type. It leave me suspicious.

@Fre4ked
Copy link

Fre4ked commented Aug 2, 2022

The return value of arrowFunctionToExpression is practical. If someone reported that their workflow is broken by this PR, we can reland it in the minor.

I can confirm that this breaks our build. If using npx-quill (and possibly many other depedencies), the .arrowFunctionToExpression({...}) returns undefined. Later on this gives a \node_modules\ngx-quill\fesm2020\ngx-quill.mjs: Cannot read property 'node' of undefined and thus the build breaks.

@JLHwung
Copy link
Contributor

JLHwung commented Aug 2, 2022

@Fre4ked Just to clarify:

Before this PR NodePath#arrowFunctionToExpression always returns undefined.
After this PR NodePath#arrowFunctionToExpression always returns a NodePath<FunctionExpression>.

Based the provided error message, I can't tell how the new behaviour will break your build. Can you open a new issue and provide a reproduction repo?

@Fre4ked
Copy link

Fre4ked commented Aug 2, 2022

The error happens when path.node is accessed in line 101. Previously the undefined result wasn't accessed later on. In the new version, the result of calling arrowFunctionToExpression is saved as path, overwriting it, meaning path is now undefined and accessing the node property throws an exception.

edit: I'm short on time coming days. I'll make a reproduction repo this weekend if it's still wanted.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Aug 3, 2022

This is a compatibility issue between new @babel/helper-wrap-function versions and old @babel/traverse versions. As a workaround, update @babel/traverse (you might need to delete it from your lockfile and re-install your dependencies)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Async Functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: _this is not defined from asyncToGenerator.js
6 participants