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

feat(transformer): complete the async-to-generator plugin #6658

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Oct 18, 2024

In this PR, most of the async functions have transformed correctly. But the async arrow functions don't fully transform correctly yet, it is related to we need to transform the arrow function to the generator function. For example:

Input:

function declaration() {
  const asy = async () => {
  console.log(this.name)
 }
}

Output:

function declaration() {
  const asy = babelHelpers.asyncToGenerator(function* () {
     console.log(this.name);
  });
}

Expected Output:

function declaration() {
  var _this = this;
  const asy = /*#__PURE__*/function () {
    var _ref = babelHelpers.asyncToGenerator(function* () {
      console.log(_this.name);
    });
    return function asy() {
      return _ref.apply(this, arguments);
    };
  }();
}

From the expected output, we haven't handled this correctly, which means even if the arrow-function plugin doesn't enable, we still need to handle this correctly as the arrow-function plugin does, and further question if arrow-function plugin is enabled, how to avoid these making conflict?

I thought we may move out the implementation of arrow-function and as a common helper, this way every plugin can handle this well

Copy link

graphite-app bot commented Oct 18, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler labels Oct 18, 2024
Copy link
Member Author

Dunqing commented Oct 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the C-enhancement Category - New feature or request label Oct 18, 2024
Copy link

codspeed-hq bot commented Oct 18, 2024

CodSpeed Performance Report

Merging #6658 will degrade performances by 6.29%

Comparing 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments (0d0bb17) with main (90c786c)

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments Change
transformer[cal.com.tsx] 30.7 ms 32.8 ms -6.29%

@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch 7 times, most recently from fd51a01 to 4db28a4 Compare October 22, 2024 08:22
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch 2 times, most recently from b9da8f5 to 3a2051e Compare October 22, 2024 12:03
@github-actions github-actions bot added the A-semantic Area - Semantic label Oct 22, 2024
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch 2 times, most recently from 05745f0 to 3e43a15 Compare October 22, 2024 12:55
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch 7 times, most recently from a1b0d3e to c80323a Compare October 23, 2024 07:05
@Dunqing Dunqing changed the title feat(transformer/async-to-generator): transform functions and arrow functions which have no id and no arguments feat(transformer): complete the async-to-generator function Oct 23, 2024
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch from c80323a to d1cdbdd Compare October 23, 2024 07:17
@Dunqing Dunqing changed the title feat(transformer): complete the async-to-generator function feat(transformer): complete the async-to-generator plugin Oct 23, 2024
overlookmotel pushed a commit that referenced this pull request Oct 23, 2024
Part of #6658

This API is useful when we want to move a scope to another scope
overlookmotel pushed a commit that referenced this pull request Oct 23, 2024
Part of #6658

This API is useful when we want to move a scope to another scope
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch 3 times, most recently from 3d212e9 to 410d5bb Compare October 24, 2024 04:00
@Dunqing Dunqing removed the A-semantic Area - Semantic label Oct 24, 2024
@Dunqing Dunqing marked this pull request as ready for review October 24, 2024 05:07
@overlookmotel
Copy link
Collaborator

Oh I'm sorry. #6856 or #6858 has caused merge conflicts and likely broken things. I should have reviewed and merged this PR first.

Copy link
Collaborator

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

I've not reviewed the logic of this transform, as I don't know it well enough. But this PR makes the tests pass so 👍!

All my comments are style nits, and some notes about use of #[inline]. I hope that's useful, and not annoying!

crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/es2017/async_to_generator.rs Outdated Show resolved Hide resolved
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch from 3c3b956 to afdb90b Compare October 25, 2024 03:02
@Dunqing
Copy link
Member Author

Dunqing commented Oct 25, 2024

Mering this!

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Oct 25, 2024
Copy link
Member Author

Dunqing commented Oct 25, 2024

Merge activity

  • Oct 24, 11:18 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Oct 24, 11:18 PM EDT: A user added this pull request to the Graphite merge queue.
  • Oct 24, 11:27 PM EDT: A user merged this pull request with the Graphite merge queue.

Dunqing added a commit that referenced this pull request Oct 25, 2024
In this PR, most of the async functions have transformed correctly. But the async arrow functions don't fully transform correctly yet, it is related to we need to transform the arrow function to the generator function. For example:

Input:
```js
function declaration() {
  const asy = async () => {
  console.log(this.name)
 }
}
```

Output:
```js
function declaration() {
  const asy = babelHelpers.asyncToGenerator(function* () {
     console.log(this.name);
  });
}
```

Expected Output:

```js
function declaration() {
  var _this = this;
  const asy = /*#__PURE__*/function () {
    var _ref = babelHelpers.asyncToGenerator(function* () {
      console.log(_this.name);
    });
    return function asy() {
      return _ref.apply(this, arguments);
    };
  }();
}
```

From the expected output, we haven't handled `this` correctly, which means even if the `arrow-function` plugin doesn't enable, we still need to handle this correctly as the `arrow-function` plugin does, and further question if `arrow-function` plugin is enabled, how to avoid these making conflict?

I thought we may move out the implementation of `arrow-function` and as a common helper, this way every plugin can handle this well
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch from afdb90b to d145f22 Compare October 25, 2024 03:19
In this PR, most of the async functions have transformed correctly. But the async arrow functions don't fully transform correctly yet, it is related to we need to transform the arrow function to the generator function. For example:

Input:
```js
function declaration() {
  const asy = async () => {
  console.log(this.name)
 }
}
```

Output:
```js
function declaration() {
  const asy = babelHelpers.asyncToGenerator(function* () {
     console.log(this.name);
  });
}
```

Expected Output:

```js
function declaration() {
  var _this = this;
  const asy = /*#__PURE__*/function () {
    var _ref = babelHelpers.asyncToGenerator(function* () {
      console.log(_this.name);
    });
    return function asy() {
      return _ref.apply(this, arguments);
    };
  }();
}
```

From the expected output, we haven't handled `this` correctly, which means even if the `arrow-function` plugin doesn't enable, we still need to handle this correctly as the `arrow-function` plugin does, and further question if `arrow-function` plugin is enabled, how to avoid these making conflict?

I thought we may move out the implementation of `arrow-function` and as a common helper, this way every plugin can handle this well
@Dunqing Dunqing force-pushed the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch from d145f22 to 0d0bb17 Compare October 25, 2024 03:23
@graphite-app graphite-app bot merged commit 0d0bb17 into main Oct 25, 2024
27 checks passed
@graphite-app graphite-app bot deleted the 10-18-feat_transformer_async-to-generator_transform_functions_and_arrow_functions_which_have_no_id_and_no_arguments branch October 25, 2024 03:27
Boshen added a commit that referenced this pull request Oct 26, 2024
## [0.34.0] - 2024-10-26

- 4618aa2 transformer: [**BREAKING**] Rename `TransformerOptions::react`
to `jsx` (#6888) (Boshen)

- 90c786c regular_expression: [**BREAKING**] Support ES2025 Duplicated
named capture groups (#6847) (leaysgur)

- 67a7bde napi/parser: [**BREAKING**] Add typings to napi/parser (#6796)
(ottomated)

### Features

- 1145341 ast_tools: Output typescript to a separate package (#6755)
(ottomated)
- 4429754 ecmascript: Constant eval `null` to number (#6879) (Boshen)
- fd57e00 ecmascript: Add abstract_relational_comparison to dce (#6846)
(Boshen)
- 8bcaf59 minifier: Late peeophole optimization (#6882) (Boshen)
- 860cbca minifier: Implement folding simple arrow fns (#6875) (camc314)
- c26020e minifier: Implement folding String.prototype.replaceAll
(#6871) (camc314)
- 50744f3 minifier: Implement folding String.prototype.replace (#6870)
(camc314)
- fccf82e minifier: Implement folding `substring` string fns (#6869)
(camc314)
- e6a5a1b minifier: Implement folding `charCodeAt` string fns (#6475)
(camc314)
- 0d0bb17 transformer: Complete the async-to-generator plugin (#6658)
(Dunqing)
- 419343b traverse: Implement `GetAddress` for `Ancestor` (#6877)
(overlookmotel)

### Bug Fixes

- a47c70e minifier: Fix remaining runtime bugs (#6855) (Boshen)
- 686727f minifier: Reference read has side effect (#6851) (Boshen)
- c658d93 minifier: Keep template literals with expressions (#6849)
(Boshen)
- 4dc5e51 transformer: Only run typescript plugin for typescript source
(#6889) (Boshen)
- 076f5c3 transformer/typescript: Retain ExportNamedDeclaration without
specifiers and declaration (#6848) (Dunqing)
- b075982 types: Change @oxc/types package name (#6874) (ottomated)

### Documentation

- 6eeb0e6 ast: Mention typescript-eslint, field ordering and shape
(#6863) (Boshen)
- 99e3b32 napi: Remove JSON.parse from example (#6836) (ottomated)

### Refactor

- adb5039 allocator: Add `impl GetAddress for Address` (#6891)
(overlookmotel)
- 3e7507f ast_tools: Reduce macro usage (#6895) (overlookmotel)
- 423d54c rust: Remove the annoying `clippy::wildcard_imports` (#6860)
(Boshen)
- 2d95009 transformer: Implement `Debug` on `StatementInjector` internal
types (#6886) (overlookmotel)
- c383c34 transformer: Make `StatementInjectorStore` methods generic
over `GetAddress` (#6885) (overlookmotel)
- 1f29523 transformer: Rename ReactJsx to Jsx (#6883) (Boshen)
- 333b758 transformer: `StatementInjectorStore` methods take
`&Statement` as target (#6858) (overlookmotel)
- c19996c transformer: Add `StatementInjectorStore::insert_many_before`
method (#6857) (overlookmotel)
- 7339dde transformer: `StatementInjectorStore::insert_many_after` take
an iterator (#6856) (overlookmotel)
- 4348eae transformer/typescript: Re-order visitor methods (#6864)
(overlookmotel)
- 3a56d59 transformer/typescript: Insert assignments after super by
`StatementInjector` (#6654) (Dunqing)
- a366fae traverse: Rename
`TraverseScoping::generate_binding_in_current_scope` (#6832)
(overlookmotel)
- 3b99fe6 traverse: Move `generate_binding` to `TraverseScoping` (#6831)
(overlookmotel)
- 60f487a traverse: `TraverseCtx::generate_binding` take an `Atom`
(#6830) (overlookmotel)

### Styling

- 262b2ed ast: Move crate doc comment to top of file (#6890)
(overlookmotel)

---------

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Orenbek pushed a commit to Orenbek/oxc that referenced this pull request Oct 28, 2024
…t#6658)

In this PR, most of the async functions have transformed correctly. But the async arrow functions don't fully transform correctly yet, it is related to we need to transform the arrow function to the generator function. For example:

Input:
```js
function declaration() {
  const asy = async () => {
  console.log(this.name)
 }
}
```

Output:
```js
function declaration() {
  const asy = babelHelpers.asyncToGenerator(function* () {
     console.log(this.name);
  });
}
```

Expected Output:

```js
function declaration() {
  var _this = this;
  const asy = /*#__PURE__*/function () {
    var _ref = babelHelpers.asyncToGenerator(function* () {
      console.log(_this.name);
    });
    return function asy() {
      return _ref.apply(this, arguments);
    };
  }();
}
```

From the expected output, we haven't handled `this` correctly, which means even if the `arrow-function` plugin doesn't enable, we still need to handle this correctly as the `arrow-function` plugin does, and further question if `arrow-function` plugin is enabled, how to avoid these making conflict?

I thought we may move out the implementation of `arrow-function` and as a common helper, this way every plugin can handle this well
Orenbek pushed a commit to Orenbek/oxc that referenced this pull request Oct 28, 2024
## [0.34.0] - 2024-10-26

- 4618aa2 transformer: [**BREAKING**] Rename `TransformerOptions::react`
to `jsx` (oxc-project#6888) (Boshen)

- 90c786c regular_expression: [**BREAKING**] Support ES2025 Duplicated
named capture groups (oxc-project#6847) (leaysgur)

- 67a7bde napi/parser: [**BREAKING**] Add typings to napi/parser (oxc-project#6796)
(ottomated)

### Features

- 1145341 ast_tools: Output typescript to a separate package (oxc-project#6755)
(ottomated)
- 4429754 ecmascript: Constant eval `null` to number (oxc-project#6879) (Boshen)
- fd57e00 ecmascript: Add abstract_relational_comparison to dce (oxc-project#6846)
(Boshen)
- 8bcaf59 minifier: Late peeophole optimization (oxc-project#6882) (Boshen)
- 860cbca minifier: Implement folding simple arrow fns (oxc-project#6875) (camc314)
- c26020e minifier: Implement folding String.prototype.replaceAll
(oxc-project#6871) (camc314)
- 50744f3 minifier: Implement folding String.prototype.replace (oxc-project#6870)
(camc314)
- fccf82e minifier: Implement folding `substring` string fns (oxc-project#6869)
(camc314)
- e6a5a1b minifier: Implement folding `charCodeAt` string fns (oxc-project#6475)
(camc314)
- 0d0bb17 transformer: Complete the async-to-generator plugin (oxc-project#6658)
(Dunqing)
- 419343b traverse: Implement `GetAddress` for `Ancestor` (oxc-project#6877)
(overlookmotel)

### Bug Fixes

- a47c70e minifier: Fix remaining runtime bugs (oxc-project#6855) (Boshen)
- 686727f minifier: Reference read has side effect (oxc-project#6851) (Boshen)
- c658d93 minifier: Keep template literals with expressions (oxc-project#6849)
(Boshen)
- 4dc5e51 transformer: Only run typescript plugin for typescript source
(oxc-project#6889) (Boshen)
- 076f5c3 transformer/typescript: Retain ExportNamedDeclaration without
specifiers and declaration (oxc-project#6848) (Dunqing)
- b075982 types: Change @oxc/types package name (oxc-project#6874) (ottomated)

### Documentation

- 6eeb0e6 ast: Mention typescript-eslint, field ordering and shape
(oxc-project#6863) (Boshen)
- 99e3b32 napi: Remove JSON.parse from example (oxc-project#6836) (ottomated)

### Refactor

- adb5039 allocator: Add `impl GetAddress for Address` (oxc-project#6891)
(overlookmotel)
- 3e7507f ast_tools: Reduce macro usage (oxc-project#6895) (overlookmotel)
- 423d54c rust: Remove the annoying `clippy::wildcard_imports` (oxc-project#6860)
(Boshen)
- 2d95009 transformer: Implement `Debug` on `StatementInjector` internal
types (oxc-project#6886) (overlookmotel)
- c383c34 transformer: Make `StatementInjectorStore` methods generic
over `GetAddress` (oxc-project#6885) (overlookmotel)
- 1f29523 transformer: Rename ReactJsx to Jsx (oxc-project#6883) (Boshen)
- 333b758 transformer: `StatementInjectorStore` methods take
`&Statement` as target (oxc-project#6858) (overlookmotel)
- c19996c transformer: Add `StatementInjectorStore::insert_many_before`
method (oxc-project#6857) (overlookmotel)
- 7339dde transformer: `StatementInjectorStore::insert_many_after` take
an iterator (oxc-project#6856) (overlookmotel)
- 4348eae transformer/typescript: Re-order visitor methods (oxc-project#6864)
(overlookmotel)
- 3a56d59 transformer/typescript: Insert assignments after super by
`StatementInjector` (oxc-project#6654) (Dunqing)
- a366fae traverse: Rename
`TraverseScoping::generate_binding_in_current_scope` (oxc-project#6832)
(overlookmotel)
- 3b99fe6 traverse: Move `generate_binding` to `TraverseScoping` (oxc-project#6831)
(overlookmotel)
- 60f487a traverse: `TraverseCtx::generate_binding` take an `Atom`
(oxc-project#6830) (overlookmotel)

### Styling

- 262b2ed ast: Move crate doc comment to top of file (oxc-project#6890)
(overlookmotel)

---------

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
overlookmotel pushed a commit that referenced this pull request Oct 31, 2024
Passed 15/19 tests. The remaining 4 failed tests related to `this` expression, the problem same as I mentioned in #6658. I will fix them in follow-up PRs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-ast Area - AST A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants