fix(transformer/top-level-statements): should not inject statements after non-import statement

[Dunqing]

• close transformer: jsx refresh injects variable after call for HOCs #12460

Injecting top-level statements after the last import statement is unsound because once there is an import statement in the last line, it would be broken if other code relies on the injected statements. Learn more to see the following example.

I suppose the reason we did this is that we had a test case that misled us. I overrode that test with the expected output.

For example:

Input:

import { observer } from 'mobx-react-lite'
import { useFoo } from './useFoo'

export const BazComponent = observer(function BazComponent_() {
  const foo = useFoo()
  return (
    <>
      {foo}
      {bar}
    </>
  )
})

import { bar } from './bar'

Before output:

import { observer } from "mobx-react-lite";
import { useFoo } from "./useFoo";
export const BazComponent = _s(observer(_c = _s(function BazComponent_() {
        _s();
        const foo = useFoo();
        return /* @__PURE__ */ _jsxs(_Fragment, { children: [foo, bar] });
}, "useFoo{foo}", false, function() {
        return [useFoo];
})), "useFoo{foo}", false, function() {
        return [useFoo];
});
_c2 = BazComponent;
import { bar } from "./bar";
import { Fragment as _Fragment, jsxs as _jsxs } from "react/jsx-runtime";
var _s = $RefreshSig$();
var _c, _c2;
$RefreshReg$(_c, "BazComponent$observer");
$RefreshReg$(_c2, "BazComponent");

After output:

import { observer } from "mobx-react-lite";
import { useFoo } from "./useFoo";
import { Fragment as _Fragment, jsxs as _jsxs } from "react/jsx-runtime";
var _s = $RefreshSig$();
export const BazComponent = _s(observer(_c = _s(function BazComponent_() {
        _s();
        const foo = useFoo();
        return /* @__PURE__ */ _jsxs(_Fragment, { children: [foo, bar] });
}, "useFoo{foo}", false, function() {
        return [useFoo];
})), "useFoo{foo}", false, function() {
        return [useFoo];
});
_c2 = BazComponent;
import { bar } from "./bar";
var _c, _c2;
$RefreshReg$(_c, "BazComponent$observer");
$RefreshReg$(_c2, "BazComponent");

The difference is that import { Fragment as _Fragment, jsxs as _jsxs } from "react/jsx-runtime"; var _s = $RefreshSig$(); has injected after import { useFoo } from "./useFoo";

[Dunqing]

• fix(transformer/top-level-statements): should not inject statements after non-import statement #12463 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #12463 will not alter performance

_{Comparing 07-23-fix_transformer_top-level-statements_should_not_inject_statements_after_non-import_statement (7c2d2c6) with main (dee25f4)¹}

Summary

✅ 34 untouched benchmarks

Footnotes

1. No successful run was found on main (c135beb) during the generation of this report, so dee25f4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[overlookmotel]

I'm not entirely sure about this.

Polyfills

The test case that's overridden here does have a rationale for why it is as it is. I don't think it's misled us exactly.

It links to this Babel issue: babel/babel#12522

If an import low down in the file is a polyfill, the polyfill should execute before import ... from "react/jsx-runtime";, so that the polyfill is applied before react is loaded. React may need some APIs which the polyfill provides.

So this PR as it is fixes one problem, but may create another.

Strictly speaking, I think the correct behavior should be to treat import statements and other statements differently:

• import statements are added after last existing import statement.
• All other statements are added at very top of file (or after initial block of imports at top of file, if we prefer).

Or:

• Move all existing import statements up to top of file (maintaining original order).
• Insert new statements (whatever they are) after all the imports.

The latter might make for more performant code, because browser will find all the imports quickly and can start fetching them before it's finished parsing the rest of the file. But bundlers presumably already do this, so maybe it's not the job of transformer to perform such optimizations. But if it's easier for us to do it like that, I don't think it'll hurt.

CommonJS

We're probably not handling CommonJS correctly either. Presumably any requires we add should be after existing requires. I'm not sure if that's tractable though, because require can appear anywhere. From the Babel issue, it sounds like they decided it's not, and ignore that problem.

What to do?

I can see 2 options here:

1. Try one of the suggestions above (or something else that has same effect).
2. Decide that polyfills are a thing of the past, and that we can safely ignore this problem! In which case this PR is good to go.

@Dunqing what do you think?

[Boshen]

Merge activity

• Jul 24, 12:49 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jul 24, 12:49 AM UTC: Boshen added this pull request to the Graphite merge queue.
• Jul 24, 1:06 AM UTC: Merged by the Graphite merge queue.

[Dunqing]

What to do?

I can see 2 options here:

1. Try one of the suggestions above (or something else that has same effect).
2. Decide that polyfills are a thing of the past, and that we can safely ignore this problem! In which case this PR is good to go.

@Dunqing what do you think?

Wow, thank you for a deeper understanding of this.

First of off, I need to point out that the example code of your linked issue is not the same as the test being overridden.

Example of babel/babel#12522:

import 'react-app-polyfill/ie11';
import 'react-app-polyfill/stable';
import ReactDOM from 'react-dom';

ReactDOM.render(
    <p>Hello, World!</p>,
    document.getElementById('root')
);

The overridden test:

// https://github.com/babel/babel/issues/12522

ReactDOM.render(
    <p>Hello, World!</p>,
    document.getElementById('root')
);

// Imports are hoisted, so this is still ok
import 'react-app-polyfill/ie11';
import 'react-app-polyfill/stable';
import ReactDOM from 'react-dom';

There is an essential difference between them! The first case we can handle well in terms of this PR solution. But the second one, yes, we will meet a problem with that, but in my opinion, users always know the polyfill should always be put on the first line, so why I am saying that I prefer to do nothing for now.

Putting this case aside, I think the first option is nice to do, but I prefer option(2) for now. The following is what I thought on this:

1. As I said above, polyfill should always be put on the first line is common practice, so the current logic can handle this well.
2. Normally, imports always come before non-import statements, so this change has no impact whatsoever and accounts for even worst-case scenarios (issue this PR linked).
3. We always have a workaround for the problem, see transformer: jsx refresh injects variable after call for HOCs #12460 (comment).
4. We have the same output as SWC. Since SWC is a mature transformer, to some extent, we follow the same standard.

Anyway, don't get me wrong. I don't disagree with you!

[overlookmotel]

First of off, I need to point out that the example code of your linked issue is not the same as the test being overridden.

True. But the test case does relate to that issue. The babel authors amended the original test case to cover the hardest case where polyfills are imported later in the file.

Yes, it's common practice to put imports of polyfills at the top of the file. So this import order problem only occurs in an edge case where polyfills are lower down, and as you say there's a workaround. But still... this edge case is legal, and I think in an ideal world, we'd handle it correctly like Babel does.

4. We have the same output as SWC. Since SWC is a mature transformer, to some extent, we follow the same standard.

You are right that SWC does not handle it correctly in the presence of polyfills: SWC playground.

But personally I don't think we should follow SWC by default. We know SWC takes shortcuts and has edge case bugs in many places, and I've heard people complain about that. So I think we should be aiming to be more correct than SWC. In particular, some big companies care a great deal about reliability and correctness, and it's a selling point of Oxc over SWC.

So... TLDR:

In practice, the polyfill problem is going to be rare. So I agree with your arguments. I think it was right to merge this PR - it solves more problems than it creates.

I'll create a backlog issue about the polyfill edge case. We may want to address it further down the line when we have time.

[Dunqing]

First of off, I need to point out that the example code of your linked issue is not the same as the test being overridden.

True. But the test case does relate to that issue. The babel authors amended the original test case to cover the hardest case where polyfills are imported later in the file.

Yes, it's common practice to put imports of polyfills at the top of the file. So this import order problem only occurs in an edge case where polyfills are lower down, and as you say there's a workaround. But still... this edge case is legal, and I think in an ideal world, we'd handle it correctly like Babel does.

4. We have the same output as SWC. Since SWC is a mature transformer, to some extent, we follow the same standard.

You are right that SWC does not handle it correctly in the presence of polyfills: SWC playground.

But personally I don't think we should follow SWC by default. We know SWC takes shortcuts and has edge case bugs in many places, and I've heard people complain about that. So I think we should be aiming to be more correct than SWC. In particular, some big companies care a great deal about reliability and correctness, and it's a selling point of Oxc over SWC.

So... TLDR:

In practice, the polyfill problem is going to be rare. So I agree with your arguments. I think it was right to merge this PR - it solves more problems than it creates.

I'll create a backlog issue about the polyfill edge case. We may want to address it further down the line when we have time.

Thanks!

[overlookmotel]

oxc-project/backlog#169