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

traverse: insert new statements(If any) after calling each walk_statement in walk_statements #4767

Closed
Dunqing opened this issue Aug 8, 2024 · 7 comments · Fixed by #5255
Closed
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Comments

@Dunqing
Copy link
Member

Dunqing commented Aug 8, 2024

Background

Here's an example of why we need to support it

// In
function A() {}

const B = hoc(() => {})

// out
function A() {}
var _s1 = A;

const B = hoc(_s2(() => {}))

function C() {}
var _s3 = C;

Looking at the example above, we can see that we need to add the var _sX = functionName statement after the function statement, and transform arrow function expression to _sX(() => {}).

For FunctionDeclaration, we can easily do it in exit_statements, but for ArrowFunctionExpression, we need to transform it in exit_arrow_function_expression. Due to these exit_* functions execution order is different. The output will look like:

// out
function A() {}
var _s2 = A;

const B = hoc(_s1(() => {}))

function C() {}
var _s3 = C;

As you can see from the output, exit_arrow_function_expression is executed first, then exit_statements. We need to make sure that the transformation order is consistent, so we can’t use exit_statements to implement it.

Possible Solution

  1. TraverseCtx provides an API where we can store statements that we want to insert after exit_statement.
  2. Mutate upwards. Provides an API that is similar to Babel's node.insertAfter(new).
@Dunqing Dunqing added C-enhancement Category - New feature or request A-transformer Area - Transformer / Transpiler labels Aug 8, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Aug 8, 2024

One more complete example here

@Dunqing
Copy link
Member Author

Dunqing commented Aug 9, 2024

I've come up with another solution that doesn't require any changes to the traverse. But it requires AstNodeId or its workaround.

Save the statements you need to insert under the statement in a HashMap with key AstNodeId and value Vec<'a, Statement<'a>> and complete inserting in exit_statements

The implementation of exit_statements looks like this

fn exit_statements(&mut self, stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TraverseCtx<'a>) {
    let new_stmts = ctx.ast.vec();
    for statement in stmts.drain(..) {
        new_stmts.push(statement);
         // Check if there are any statements that need to be inserted below this statement. 
        if let Some(ss) = StmtsHashMap.get(statement.id) { 
            new_stmts.extend(ss)
        }
    }
}

@Dunqing Dunqing changed the title traverse: insert new statements(If exist) after calling each walk_statement in walk_statements traverse: insert new statements(If any) after calling each walk_statement in walk_statements Aug 9, 2024
@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 10, 2024

That solution sounds good. I propose that we implement #4807 to support that.

Further down the line, switching Program::body from a Vec<Statement> to ChunkedVec<Statement> (see oxc-project/backlog#35 (comment)) I'd hope would make it possible to add the top-level statement from the child visitor. But that is quite complex and will not be quick to come to fruition! So I think what you've suggested is best solution in meantime.

@overlookmotel
Copy link
Contributor

I've made a PR for Address API, which can be used for unique identifier: #4810

@Dunqing
Copy link
Member Author

Dunqing commented Aug 12, 2024

I've made a PR for Address API, which can be used for unique identifier: #4810

That's great. Should we implement this inside a specific plugin, transformer, or traverse? This functionality is useful in many plugins, such as the TypeScript enum and arrow-function plugin.

@Dunqing Dunqing mentioned this issue Aug 12, 2024
@overlookmotel
Copy link
Contributor

Actually, in this case I don't think you need either Address or AstNodeId. As the statements which need to locate in final pass are function declarations or variable declarations, I think could use SymbolId of the bindings to identify them (NB: if redeclarations are possible, would need to track when a symbol has been found already to ignore the redeclarations).

@Dunqing
Copy link
Member Author

Dunqing commented Aug 13, 2024

Actually, in this case I don't think you need either Address or AstNodeId. As the statements which need to locate in final pass are function declarations or variable declarations, I think could use SymbolId of the bindings to identify them (NB: if redeclarations are possible, would need to track when a symbol has been found already to ignore the redeclarations).

Oh, yes this is a workable approach

Dunqing added a commit that referenced this issue Aug 15, 2024
close: #3943

## Further improvements

There is a double visit here. We need to collect all react hooks calling in `Function` and `ArrowFunctionExpression`. If we want to remove this implementation, we may wait for #4188.

https://github.com/oxc-project/oxc/blob/d797e595d286c613848b773c256bd43124ad1981/crates/oxc_transformer/src/react/refresh.rs#L744-L947

## Tests

All tests copy from https://github.com/facebook/react/blob/main/packages/react-refresh/src/__tests__/ReactFresh-test.js

There are still 4 tests that have not been passed

**1. refresh/can-handle-implicit-arrow-returns/input.jsx**

Related to #4767. transform correct, just output doesn't match the expected output

**2. refresh/registers-identifiers-used-in-jsx-at-definition-site/input.jsx**
**3. refresh/registers-identifiers-used-in-react-create-element-at-definition-site/input.jsx**

Blocked by #4746

**4. refresh/supports-typescript-namespace-syntax/input.tsx**

oxc transforms ts to js first, so probably we can ignore this case. If we really want to pass this test, we also need to turn off `TypeScript` plugin.

## What's next?

### Options:

1. Support transform `refresh_reg` and `refresh_sig` options to `MemberExpression`. Currently `import.meta.xxxx` still is an `Identifier`
2. Support `emit_full_signatures` option

### Other
NAPI, testing in `monitor-oxc`, etc..
Dunqing added a commit that referenced this issue Aug 15, 2024
close: #3943

## Further improvements

There is a double visit here. We need to collect all react hooks calling in `Function` and `ArrowFunctionExpression`. If we want to remove this implementation, we may wait for #4188.

https://github.com/oxc-project/oxc/blob/d797e595d286c613848b773c256bd43124ad1981/crates/oxc_transformer/src/react/refresh.rs#L744-L947

## Tests

All tests copy from https://github.com/facebook/react/blob/main/packages/react-refresh/src/__tests__/ReactFresh-test.js

There are still 4 tests that have not been passed

**1. refresh/can-handle-implicit-arrow-returns/input.jsx**

Related to #4767. transform correct, just output doesn't match the expected output

**2. refresh/registers-identifiers-used-in-jsx-at-definition-site/input.jsx**
**3. refresh/registers-identifiers-used-in-react-create-element-at-definition-site/input.jsx**

Blocked by #4746

**4. refresh/supports-typescript-namespace-syntax/input.tsx**

oxc transforms ts to js first, so probably we can ignore this case. If we really want to pass this test, we also need to turn off `TypeScript` plugin.

## What's next?

### Options:

1. Support transform `refresh_reg` and `refresh_sig` options to `MemberExpression`. Currently `import.meta.xxxx` still is an `Identifier`
2. Support `emit_full_signatures` option

### Other
NAPI, testing in `monitor-oxc`, etc..
overlookmotel pushed a commit that referenced this issue Aug 29, 2024
@Dunqing Dunqing closed this as completed Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants