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(oxc_transformer): correct generate ThisExpr and import.meta in jsx pragma #7553

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

IWANABETHATGUY
Copy link
Contributor

No description provided.

Copy link

graphite-app bot commented Nov 30, 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.

Copy link
Contributor Author

IWANABETHATGUY commented Nov 30, 2024

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Nov 30, 2024
@IWANABETHATGUY IWANABETHATGUY changed the title fix: correct generate this expr fix(oxc_transformer): correct generate this expr and import.meta in jsx pragma Nov 30, 2024
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review November 30, 2024 10:38
@IWANABETHATGUY IWANABETHATGUY force-pushed the 11-30-fix_correct_generate_this_expr branch from 59ddf22 to 0158063 Compare November 30, 2024 10:40
@github-actions github-actions bot added the C-bug Category - Bug label Nov 30, 2024
@IWANABETHATGUY IWANABETHATGUY force-pushed the 11-30-fix_correct_generate_this_expr branch from 0158063 to 657269c Compare November 30, 2024 10:42
Copy link

codspeed-hq bot commented Nov 30, 2024

CodSpeed Performance Report

Merging #7553 will not alter performance

Comparing 11-30-fix_correct_generate_this_expr (6af8659) with main (6dd71c6)

Summary

✅ 29 untouched benchmarks

@Boshen
Copy link
Member

Boshen commented Dec 2, 2024

@overlookmotel need your review.

@IWANABETHATGUY IWANABETHATGUY changed the title fix(oxc_transformer): correct generate this expr and import.meta in jsx pragma fix(oxc_transformer): correct generate ThisExpr and import.meta in jsx pragma Dec 2, 2024
@overlookmotel overlookmotel changed the base branch from 11-29-fix_support_arbitrary_length_member_expr to graphite-base/7553 December 2, 2024 14:02
@overlookmotel overlookmotel force-pushed the 11-30-fix_correct_generate_this_expr branch from 657269c to c36fb3a Compare December 2, 2024 14:07
@overlookmotel overlookmotel changed the base branch from graphite-base/7553 to main December 2, 2024 14:08
@overlookmotel overlookmotel force-pushed the 11-30-fix_correct_generate_this_expr branch from c36fb3a to fd6f836 Compare December 2, 2024 14:08
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.

Sorry to say I see problems.

Also, could you please add a test for import.meta?

We can make this more performant by changing Pragma into an enum, to avoid string comparisons on every call to create_expression. Something like:

enum Pragma<'a> {
  /// `createElement`
  Single(Atom<'a>),
  /// `React.createElement`
  Double(Atom<'a>, Atom<'a>),
  /// `foo.bar.qux`
  Many(Vec<Atom<'a>>),
  /// `this`, `this.foo`, `this.foo.bar.qux`
  This(Vec<Atom<'a>>),
  /// `import.meta`, `import.meta.foo`, `import.meta.foo.bar.qux`
  ImportMeta(Vec<Atom<'a>>),
}

In practice, it's going to always be Single or Double. All the others are edge cases.

But we can do that optimization in a follow-up.

crates/oxc_transformer/src/jsx/jsx_impl.rs Outdated Show resolved Hide resolved
crates/oxc_transformer/src/jsx/jsx_impl.rs Outdated Show resolved Hide resolved
@IWANABETHATGUY IWANABETHATGUY force-pushed the 11-30-fix_correct_generate_this_expr branch 5 times, most recently from 9014d31 to db63e45 Compare December 3, 2024 12:39
@IWANABETHATGUY IWANABETHATGUY force-pushed the 11-30-fix_correct_generate_this_expr branch from 4e15836 to 8d4323c Compare December 3, 2024 12:57
@overlookmotel overlookmotel force-pushed the 11-30-fix_correct_generate_this_expr branch from 8ea7a7b to 62fbc1e Compare December 3, 2024 13:39
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.

Thanks for adding the tests.

I've pushed a couple more commits:

  1. Get rid of the Boxes in create_arbitrary_length_member_expr_or_ident. They didn't appear to add anything.
  2. Tweak the tests to make them more explicit.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Dec 3, 2024
Copy link
Contributor

overlookmotel commented Dec 3, 2024

Merge activity

  • Dec 3, 8:53 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 3, 8:53 AM EST: A user added this pull request to the Graphite merge queue.
  • Dec 3, 8:59 AM EST: A user merged this pull request with the Graphite merge queue.

@overlookmotel overlookmotel force-pushed the 11-30-fix_correct_generate_this_expr branch from 62fbc1e to 6af8659 Compare December 3, 2024 13:54
@graphite-app graphite-app bot merged commit 6af8659 into main Dec 3, 2024
25 checks passed
@graphite-app graphite-app bot deleted the 11-30-fix_correct_generate_this_expr branch December 3, 2024 13:59
Boshen pushed a commit that referenced this pull request Dec 4, 2024
Follow up after #7553. Move tests setup for JSX pragmas into a macro to prevent tests having access to an owned `TraverseCtx`, for safety.
Boshen pushed a commit that referenced this pull request Dec 4, 2024
…sion (#7620)

#7553 introduced support for some unusual JSX pragmas e.g. `this.foo` and `import.meta.foo`. We want to support these to pass tests, but they're very unlikely to be used in practice.

Identify these strange patterns when parsing the pragma (which happens only once per file), and encode them as an enum. The removes expensive string comparisons from `Pragma::create_expression` (which is called for every JSX element), and keeps the path for common cases fast.
@oxc-bot oxc-bot mentioned this pull request Dec 4, 2024
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-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants