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

[Compiler Todo]: Handle TSSatisfiesExpression expressions #29754

Open
4 tasks
nikeee opened this issue Jun 4, 2024 · 3 comments · May be fixed by #29818
Open
4 tasks

[Compiler Todo]: Handle TSSatisfiesExpression expressions #29754

nikeee opened this issue Jun 4, 2024 · 3 comments · May be fixed by #29818

Comments

@nikeee
Copy link
Contributor

nikeee commented Jun 4, 2024

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhHCA7MAXABAQ1wF5cBGXMfbASzADNqExcMoBbAIwRgG4AdDCAC+QA

Repro steps

The compiler currently cannot handle TSSatisfiesExpression

const a = 1 satisfies number;

Results in

(BuildHIR::lowerExpression) Handle TSSatisfiesExpression expressions

I think adding support to the code itself would be pretty straignt-forward, as the typescript construct of satisfies can just be ignored and handled as the inner expression:

    case "TSSatisfiesExpression": {
      const expr = exprPath as NodePath<t.TSSatisfiesExpression>;
      return lowerExpression(builder, expr.get("expression"));
    }

https://github.com/facebook/react/compare/main...nikeee:react:handle-satisfies-expression?expand=1

However, when implementing this, it does not build, due to type conflicts of @babel/types. npm ls @babel/types suggests that there are multiple versions of @babel/types in use, some of which already support satisfies and others don't. satisfies was introduced in TS 4.9, which is supported since babel 7.20.0.
The RC itself does depend on a lower version, but ^7.20 versions are included as a transitive dependency. This might be the reason why the parser does not throw an error, while RC cannot handle it (and BuildHIR doesn't know anything about that).
I also tried upgrading babel oin the entire project, but that seems to be not as trivial as I'd thought. Maybe there is a different solution.

How often does this bug happen?

Every time

What version of React are you using?

19

@nikeee nikeee added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels Jun 4, 2024
@josephsavona
Copy link
Contributor

For some internal use-cases we need to support older versions of Babel, so unfortunately we can't just upgrade to 7.20. However we can align on a more recent version of @babel/types that does support this ast type, and then handle it as you suggested. Open to PRs!

@josephsavona josephsavona added Compiler: todo and removed Type: Bug Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Jun 4, 2024
@josephsavona
Copy link
Contributor

Note that all errors categorized as todo are places where the compiler knowingly does not support a feature. We don't categorize those as bugs because the compiler is safely skipping compilation of something it can't compile. In general we're open to PRs to add support for additional syntax as long as its part of JS, TS, or Flow. There are some exceptions — for example, we probably won't ever support nested class declarations within components, bc the complexity cost is high enough and it's exceedingly rare. But for everday, uncomplicated things like sastisfies we're open to PRs.

@rodrigofariow
Copy link

rodrigofariow commented Jun 5, 2024

For those that may want to try to "patch" this I used @nikeee gist and pnpm patched babel-plugin-react-compiler with it.

Everything seems to work and according to @josephsavona it should be mostly fine? Works for my project 🥳

Cheers

@nikeee nikeee linked a pull request Jun 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants