feat(transformer): support react fast refresh

[Dunqing]

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.

oxc/crates/oxc_transformer/src/react/refresh.rs

Lines 744 to 947 in d797e59

	// TODO: Try to remove this struct, avoid double visiting
	struct CalculateSignatureKey<'a, 'b> {
	key: String,
	source_text: &'a str,
	ctx: &'b mut TraverseCtx<'a>,
	callee_list: Vec<(Atom<'a>, Option<Atom<'a>>)>,
	scope_ids: Vec<ScopeId>,
	declarator_id_span: Option<Span>,
	}
	impl<'a, 'b> CalculateSignatureKey<'a, 'b> {
	pub fn new(source_text: &'a str, scope_id: ScopeId, ctx: &'b mut TraverseCtx<'a>) -> Self {
	Self {
	key: String::new(),
	ctx,
	source_text,
	scope_ids: vec![scope_id],
	declarator_id_span: None,
	callee_list: Vec::new(),
	}
	}
	fn current_scope_id(&self) -> ScopeId {
	*self.scope_ids.last().unwrap()
	}
	pub fn calculate(
	mut self,
	body: &FunctionBody<'a>,
	) -> Option<oxc_allocator::Vec<'a, Argument<'a>>> {
	for statement in &body.statements {
	self.visit_statement(statement);
	}
	if self.key.is_empty() {
	return None;
	}
	// Check if a corresponding binding exists where we emit the signature.
	let mut force_reset = false;
	let mut custom_hooks_in_scope = self.ctx.ast.vec_with_capacity(self.callee_list.len());
	for (binding_name, hook_name) in &self.callee_list {
	if let Some(symbol_id) =
	self.ctx.scopes().find_binding(self.ctx.current_scope_id(), binding_name)
	{
	let ident = self.ctx.create_reference_id(
	SPAN,
	binding_name.clone(),
	Some(symbol_id),
	ReferenceFlag::Read,
	);
	let mut expr = self.ctx.ast.expression_from_identifier_reference(ident);
	if let Some(hook_name) = hook_name {
	// binding_name.hook_name
	expr = Expression::from(self.ctx.ast.member_expression_static(
	SPAN,
	expr,
	self.ctx.ast.identifier_name(SPAN, hook_name),
	false,
	));
	}
	custom_hooks_in_scope.push(self.ctx.ast.array_expression_element_expression(expr));
	} else {
	force_reset = true;
	}
	}
	let mut arguments = self.ctx.ast.vec_with_capacity(
	1 + usize::from(force_reset) + usize::from(!custom_hooks_in_scope.is_empty()),
	);
	arguments.push(self.ctx.ast.argument_expression(
	self.ctx.ast.expression_string_literal(SPAN, self.ctx.ast.atom(&self.key)),
	));
	if force_reset || !custom_hooks_in_scope.is_empty() {
	arguments.push(
	self.ctx.ast.argument_expression(
	self.ctx.ast.expression_boolean_literal(SPAN, force_reset),
	),
	);
	}
	if !custom_hooks_in_scope.is_empty() {
	// function () { return custom_hooks_in_scope }
	let formal_parameters = self.ctx.ast.formal_parameters(
	SPAN,
	FormalParameterKind::FormalParameter,
	self.ctx.ast.vec(),
	Option::<BindingRestElement>::None,
	);
	let function_body = self.ctx.ast.function_body(
	SPAN,
	self.ctx.ast.vec(),
	self.ctx.ast.vec1(self.ctx.ast.statement_return(
	SPAN,
	Some(self.ctx.ast.expression_array(SPAN, custom_hooks_in_scope, None)),
	)),
	);
	let fn_expr = self.ctx.ast.expression_function(
	FunctionType::FunctionExpression,
	SPAN,
	None,
	false,
	false,
	false,
	Option::<TSTypeParameterDeclaration>::None,
	Option::<TSThisParameter>::None,
	formal_parameters,
	Option::<TSTypeAnnotation>::None,
	Some(function_body),
	);
	arguments.push(self.ctx.ast.argument_expression(fn_expr));
	}
	Some(arguments)
	}
	}
	impl<'a, 'b> Visit<'a> for CalculateSignatureKey<'a, 'b> {
	fn enter_scope(&mut self, _flags: ScopeFlags, scope_id: &Cell<Option<oxc_semantic::ScopeId>>) {
	self.scope_ids.push(scope_id.get().unwrap());
	}
	fn leave_scope(&mut self) {
	self.scope_ids.pop();
	}
	fn visit_statements(&mut self, _stmt: &oxc_allocator::Vec<'a, Statement<'a>>) {
	// We don't need calculate any signature in nested scopes
	}
	fn visit_variable_declarator(&mut self, declarator: &VariableDeclarator<'a>) {
	if matches!(declarator.init, Some(Expression::CallExpression(_))) {
	self.declarator_id_span = Some(declarator.id.span());
	}
	walk_variable_declarator(self, declarator);
	// We doesn't check the call expression is the hook,
	// So we need to reset the declarator_id_span after visiting the variable declarator.
	self.declarator_id_span = None;
	}
	fn visit_call_expression(&mut self, call_expr: &CallExpression<'a>) {
	if !self.ctx.scopes().get_flags(self.current_scope_id()).is_function() {
	return;
	}
	let name = match &call_expr.callee {
	Expression::Identifier(ident) => Some(ident.name.clone()),
	Expression::StaticMemberExpression(ref member) => Some(member.property.name.clone()),
	_ => None,
	};
	let Some(hook_name) = name else {
	return;
	};
	if !is_use_hook_name(&hook_name) {
	return;
	}
	if !is_builtin_hook(&hook_name) {
	let callee = match &call_expr.callee {
	Expression::Identifier(ident) => Some((ident.name.clone(), None)),
	callee @ match_member_expression!(Expression) => {
	let member_expr = callee.to_member_expression();
	match member_expr.object() {
	Expression::Identifier(ident) => {
	Some((ident.name.clone(), Some(hook_name.clone())))
	}
	_ => None,
	}
	}
	_ => None,
	};
	if let Some(callee) = callee {
	self.callee_list.push(callee);
	}
	}
	let args = &call_expr.arguments;
	let args_key = if hook_name == "useState" && args.len() > 0 {
	args[0].span().source_text(self.source_text)
	} else if hook_name == "useReducer" && args.len() > 1 {
	args[1].span().source_text(self.source_text)
	} else {
	""
	};
	if !self.key.is_empty() {
	self.key.push_str("\\n");
	}
	self.key.push_str(&format!(
	"{hook_name}{{{}{}{args_key}{}}}",
	self.declarator_id_span.take().map_or("", |span| span.source_text(self.source_text)),
	if args_key.is_empty() { "" } else { "(" },
	if args_key.is_empty() { "" } else { ")" }
	));
	}
	}

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]

• feat(transformer): support react fast refresh #4587 👈
• main

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

Join @Dunqing and the rest of your teammates on Graphite

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “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.

[codspeed-hq]

CodSpeed Performance Report

Merging #4587 will not alter performance

_{Comparing 07-31-feat_transformer_support_react_fast_refresh (f1fcdde) with main (b3ec9e5)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Is this ready for review? Or blocked on #4746 and #4767?

[Boshen]

Is this ready for review? Or blocked on #4746 and #4767?

Ready for review. Can be merged before fixing these.

[overlookmotel]

As discussed, I haven't gone fully into this, so I haven't sought to understand all the logic of the plugin, or this implementation. But I get at least a vague sense that maybe there's some duplicated logic in this implementation, especially in transform_statements / transform_statements_on_exit.

I may be wrong about that, but could you please check?

Just to be clear, I don't object to merging this - we could always merge and then iterate. But you've asked me to review, so I'm just giving my thoughts.

Wider point: I think we need a better naming convention to make it easier to recognise which functions are "entry points" from the visitor, and which are functions which are only called by those entry points. I think if we call the entry points transform_* and transform_*_on_exit, we should use different names for the internal methods. When everything is called transform_*, it makes it quite hard to follow the logic.

[Dunqing]

Wider point: I think we need a better naming convention to make it easier to recognise which functions are "entry points" from the visitor, and which are functions which are only called by those entry points. I think if we call the entry points transform_* and transform_on_exit, we should use different names for the internal methods. When everything is called transform, it makes it quite hard to follow the logic.

Yes! I agree, I'll try to rename these methods

[Dunqing]

@overlookmotel Please take a look again, once this is merged, I will start to refactor it according to #4881 and try to resolve the remaining tests

[overlookmotel]

I can't review this properly without delving fully into how the transform works.

I agree that we should merge it now. If you're going to refactor it anyway in a follow-on, it'll be easier for me to do a "2nd pass" review after that.

Currently there are merge conflicts. If you're able to resolve those, please go ahead and merge.

[graphite-app]

Merge activity

• Aug 15, 12:38 PM EDT: Dunqing added this pull request to the Graphite merge queue.
• Aug 15, 12:45 PM EDT: Dunqing merged this pull request with the Graphite merge queue.