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

test(transformer/class-properties): add static super tagged template test #7964

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Dec 17, 2024

I just found that we don't need to transform TaggedTemplateExpression because its tag can be transformed by transform_static_member_expression and transform_computed_member_expression

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-test Category - Testing. Code is missing test cases, or a PR is adding them labels Dec 17, 2024
Copy link
Member Author

Dunqing commented Dec 17, 2024


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@Dunqing Dunqing changed the title test(transformer/class-properties): add static super tagged expression test test(transformer/class-properties): add static super tagged template test Dec 17, 2024
@overlookmotel
Copy link
Contributor

overlookmotel commented Dec 17, 2024

Unfortunately this is a bug in Babel (another one!)

import assert from 'assert';

let receiver;
class S {
  static method() {
    receiver = this;
  }
}

class C extends S {
  static prop = super.method`xyz`;
}

assert(receiver === C);

The assertion passes.

After Babel transform:

import assert from "assert";

let receiver;
class S {
  static method() {
    receiver = this;
  }
}

class C extends S {}
_C = C;
_defineProperty(C, "prop", _superPropGet(_C, "method", _C)`xyz`);

assert(receiver === C);

Assertion fails!

Same as for private field as template tag, it needs extra .bind(_C):

_defineProperty(C, "prop", _superPropGet(_C, "method", _C).bind(_C)`xyz`);

Babel REPL

I will add this to my list of bugs to report to Babel!

We don't need to fix this right now. It's a crazy edge case.

But I don't think we should add a test with wrong output either. Maybe better to adapt this test to be a failing test, so we remember to come back to it later.

@Dunqing
Copy link
Member Author

Dunqing commented Dec 18, 2024

Thank you for finding the bug, I changed the output to the correct output and also added an execution test from your example

@Dunqing Dunqing force-pushed the 12-17-test_transformer_class-properties_add_static_super_tagged_expression_test branch 2 times, most recently from 2c21608 to 462af93 Compare December 18, 2024 05:50
Copy link
Contributor

@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.

Great. Your version of the exec test is better than mine.

I pushed a commit to remove the 2nd test case, which had the wrong output. Is there a reason that was there that I don't understand?

Also:

  • Removed import { expect } from "vitest"; - test runner already injects expect into environment.
  • Made input.js a copy of exec.js. This follows what Babel does. I guess the reason is that when debugging the exec test, it's helpful to be able to see the transformed output.

@Dunqing If there wasn't a good reason why the 2nd test was there, please merge. If there was a good reason, and I shouldn't have removed it, please say!

@Dunqing
Copy link
Member Author

Dunqing commented Dec 18, 2024

@Dunqing If there wasn't a good reason why the 2nd test was there, please merge. If there was a good reason, and I shouldn't have removed it, please say!

It's a accident, it seemed caused by renaming the test folder name.

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

Dunqing commented Dec 18, 2024

Merge activity

  • Dec 18, 5:24 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 18, 5:24 AM EST: A user added this pull request to the Graphite merge queue.
  • Dec 18, 5:29 AM EST: A user merged this pull request with the Graphite merge queue.

…test (#7964)

I just found that we don't need to transform `TaggedTemplateExpression` because its `tag` can be transformed by `transform_static_member_expression` and `transform_computed_member_expression`
@Dunqing Dunqing force-pushed the 12-17-test_transformer_class-properties_add_static_super_tagged_expression_test branch from ba041f3 to e3d0889 Compare December 18, 2024 10:25
@graphite-app graphite-app bot merged commit e3d0889 into main Dec 18, 2024
16 checks passed
@graphite-app graphite-app bot deleted the 12-17-test_transformer_class-properties_add_static_super_tagged_expression_test branch December 18, 2024 10:29
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-transformer Area - Transformer / Transpiler C-test Category - Testing. Code is missing test cases, or a PR is adding them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants