traverse: support for getting AstNode by AstNodeId

[Dunqing]

related: #4188

I am working on #3943. We need to port a logic from Babel's version plugin, but currently, we can't get the reference node by reference's stored node id. We don't seem to have a workaround to do it right now.

There are two tests related to the above logic here.
https://github.com/facebook/react/blob/ba6a9e94edf0db3ad96432804f9931ce9dc89fec/packages/react-refresh/src/__tests__/ReactFreshBabelPlugin-test.js#L253-L333

[overlookmotel]

I'm sorry to say that what you're asking for is currently impossible in Traverse (and also VisitMut).

Visit can do this because it is dealing with an immutable AST, so it's no problem to get multiple overlapping & refs to AST nodes at the same time. But in Traverse and Visit, in a visitor you hold a &mut ref to an AST node, so it would be a violation of Rust's aliasing rules to also hold a & ref to a child node. For this reason, Traverse and VisitMut cannot get a node from an AstNodeId.

I can see a possible new API for Traverse which would make this possible by forcing you to "swap" the &mut reference you hold for a &mut ref to another AST node (so you only hold one at any time). But that is quite fraught with danger! I'm not completely sure it can be done safely and, even if it can, it would be a sizeable job to overhaul Traverse to enable it. So I'm afraid it won't be coming in near future.

Is there any other way to make this transform work within this restriction?

[Dunqing]

Is there any other way to make this transform work within this restriction?

There doesn't seem to be a good way to do this at the moment, it's almost like reimplementing ScopeTree and SymbolTable all over again! We don't need to fix this right away, we can do it before milestone 2!

[overlookmotel]

I think maybe this would work:

• During traversal, collect the SymbolIds of all references which appear in <Foo> or React.createElement(Foo, ...) in exit_identifier_reference and exit_jsx_opening_element visitors. i.e. make a list of variables which are used as React components.
• Do all the mutation in exit_statements, where can check against this collected list to determine if a binding is a React component or not.

At present, JSXIdentifier doesn't have a reference_id field, but it should (#3528), and then the above should work I think. It will also work now if JSX->JS transform happens before this transform, as that converts Foo in <Foo> to an IdentifierReference which does have a reference_id.

Do you think it can work to move all the "action" into exit_statements?

[Dunqing]

I think maybe this would work:

• During traversal, collect the SymbolIds of all references which appear in <Foo> or React.createElement(Foo, ...) in exit_identifier_reference and exit_jsx_opening_element visitors. i.e. make a list of variables which are used as React components.
• Do all the mutation in exit_statements, where can check against this collected list to determine if a binding is a React component or not.

At present, JSXIdentifier doesn't have a reference_id field, but it should (#3528), and then the above should work I think. It will also work now if JSX->JS transform happens before this transform, as that converts Foo in <Foo> to an IdentifierReference which does have a reference_id.

Sounds like it would be work

Do you think it can work to move all the "action" into exit_statements?

I'm not sure I get your meaning; could you please explain further?

[overlookmotel]

When I said "move all the action" I meant: do all mutating of the AST in one place in exit_statements. If I've understood correctly, by the time you get to exit_statements, I think you'd know which bindings are being used as React components and which aren't.

Sorry I wasn't very clear!

[overlookmotel]

@Dunqing I think the original reason for this issue - the problem in React Refresh transform - is now solved. If so, can we close this issue, and continue tracking adding AstNodeIds to AST nodes in #4188?

[Dunqing]

@Dunqing I think the original reason for this issue - the problem in React Refresh transform - is now solved. If so, can we close this issue, and continue tracking adding AstNodeIds to AST nodes in #4188?

Unfortunately, it still hasn't been resolved.

I think maybe this would work:

During traversal, collect the SymbolIds of all references which appear in or React.createElement(Foo, ...) in exit_identifier_reference and exit_jsx_opening_element visitors. i.e. make a list of variables which are used as React components.
Do all the mutation in exit_statements, where can check against this collected list to determine if a binding is a React component or not.

I've tested this solution, but unfortunately, it doesn't work. We can't move the mutation from enter_program to exit_program because the refresh plugin performs two conflicting transformations.

[Dunqing]

I've thought of two solutions inspired by you:

1. Perform an additional AST pass to collect JSX references.
2. Add a ReferenceFlags::JSX to indicate the binding referenced by JSX

To be honest, both of the solutions are not good.
The first one should seriously cause a performance impact.
The second one relies on Semantic to create a backdoor for the transformer.

[overlookmotel]

Oh damn, I thought this was solved now.

I've thought of two solutions inspired by you:

1. Perform an additional AST pass to collect JSX references.
2. Add a ReferenceFlags::JSX to indicate the binding referenced by JSX

Glad to hear I inspire bad solutions! 😄

Yes, I'm afraid I don't think either of those solutions is great. Concerning the ReferenceFlags one, I feel that the array of flags we have already (especially SymbolFlags) are much too sprawling, and ideally I'd like to see less flags, rather than more.

But I'm not really on top of the React Refresh transform, so I don't have any bright ideas for better solutions at present. When I finally get to look at it properly and understand the logic, maybe I will do. Maybe...

[Dunqing]

Yes, I'm afraid I don't think either of those solutions is great. Concerning the ReferenceFlags one, I feel that the array of flags we have already (especially SymbolFlags) are much too sprawling, and ideally I'd like to see less flags, rather than more.

I agree with you on this point.

But I'm not really on top of the React Refresh transform, so I don't have any bright ideas for better solutions at present. When I finally get to look at it properly and understand the logic, maybe I will do. Maybe...

Yes, the implementation of the plugin is very complex, and only by understanding its logic can one find a feasible solution.