oxc-project / oxc

⚓ A collection of JavaScript tools written in Rust.
https://oxc.rs
MIT License
12.58k stars 461 forks source link

transformer: React Refresh different output compared to babel #6139

Closed Boshen closed 2 months ago

Boshen commented 2 months ago

Using https://github.com/oxc-project/bench-transformer/blob/main/fixtures/UserSettings.tsx

With https://github.com/oxc-project/bench-transformer/blob/1d97626d9a40046e4be7a3538ec1a9378cdd64c5/src/transform.bench.js#L8-L9

const development = true;
const refresh = true;

Uncomment https://github.com/oxc-project/bench-transformer/blob/1d97626d9a40046e4be7a3538ec1a9378cdd64c5/src/transform.bench.js#L63

then run pnpm run bench transform

Oxc produces

_s(UserSettings, "xzl9n9mUlx5jvXUu+MIqPIqvR+E=", false, function() {
    return [
        trpc.useSuspenseQuery,
        useLocale,
        useTimePreferences,
        useTelemetry,
        useForm,
        trpc.useUtils,
        trpc.useMutation
    ];
});

Babel produces

_s(UserSettings, "t749M4ZN74U/lulDMtBAO8yAXBg=", true, function () {
  return [useLocale, useTimePreferences, useTelemetry, useForm, trpc.useUtils];
});

swc produces

_s(UserSettings, "t749M4ZN74U/lulDMtBAO8yAXBg=", false, function() {
    return [
        useLocale,
        useTimePreferences,
        useTelemetry,
        useForm,
        trpc.useUtils
    ];
});
Boshen commented 2 months ago

What is the 3rd argument true / false?

Dunqing commented 2 months ago

What is the 3rd argument true / false?

This argument means whether to force reset, But I don't how it does in the runtime of React Fast Refresh

https://github.com/oxc-project/oxc/blob/f468da13b8c8d7b0722afec9f232c6606ea4562c/crates/oxc_transformer/src/react/refresh.rs#L634-L640

The implementation of Babel handling here

https://github.com/facebook/react/blob/ba6a9e94edf0db3ad96432804f9931ce9dc89fec/packages/react-refresh/src/ReactFreshBabelPlugin.js#L294-L318

I re-read this logic and I haven't seen the problem. Needs to debug how was going in Babel plugin

ArnaudBarre commented 2 months ago

I think Babel logic output true because it for trpc.viewer.me.useSuspenseQuery() is analyzed as me.useSuspenseQuery() and me is not in scope. I think the ideal output in the logic of fast refresh would be to have fals and trpc.viewer.me.useSuspenseQuery in the hook array, but I think following swc behaviour is better for compatibility

Dunqing commented 2 months ago

I think Babel logic output true because it for trpc.viewer.me.useSuspenseQuery() is analyzed as me.useSuspenseQuery() and me is not in scope. I think the ideal output in the logic of fast refresh would be to have fals and trpc.viewer.me.useSuspenseQuery in the hook array, but I think following swc behaviour is better for compatibility

Thanks for your investigation. Now the output same as SWC, and then all of us implementation has some problems 😢