Babel preset fails for Vite production build when functions are hoisted within components

[devuxer]

Hey @pzuraq,

I have several components with nested functions that are defined after the return statement, and therefore rely on JS hoisting.

Example:

import Stack from "@mui/material/Stack";
import { component } from "signalium/react";
import TableView from "@/features/dashboard/TableView";
import { dashboardFormat } from "@/features/dashboard/toolbar/toolbar-state";
import { unitStates } from "./dashboard-state";
import Toolbar from "./toolbar/Toolbar";
import UnitPanel from "./unit-summary/UnitPanel";

const Dashboard = component(() => {
    return (
        <Stack spacing={2} sx={{ maxWidth: "75em" }}>
            <Toolbar />
            {getBody()}
        </Stack>
    );

    function getBody() {
        switch (dashboardFormat.value) {
            case "Card":
                return unitStates().map(x => <UnitPanel key={x.code} state={x} />);
            case "Table":
                return <TableView />;
        }
    }
});

export default Dashboard;

This has been working fine in development, but I now realize it fails in production when the Babel preset is used.

This is what gets generated after running vite build:

const Dashboard = component(() => {
  return /* @__PURE__ */ jsxRuntimeExports.jsxs(Stack, { spacing: 2, sx: {
    maxWidth: "75em"
  }, children: [
    /* @__PURE__ */ jsxRuntimeExports.jsx(Toolbar, {}),
    getBody()
  ] });
  const getBody = callback(function getBody2() {
    switch (dashboardFormat.value) {
      case "Card":
        return unitStates().map(callback((x) => /* @__PURE__ */ jsxRuntimeExports.jsx(UnitPanel, { state: x }, x.code), 0));
      case "Table":
        return /* @__PURE__ */ jsxRuntimeExports.jsx(TableView, {});
    }
  }, 0);
});

This fails because getBody() is not defined at the time it is called.

Vite config:

import react from "@vitejs/plugin-react";
import { signaliumPreset } from "signalium/transform";
import { defineConfig } from "vite";
import viteTsconfigPaths from "vite-tsconfig-paths";

// https://vite.dev/config/
export default defineConfig({
    plugins: [react({ babel: { presets: [signaliumPreset()] } }), viteTsconfigPaths()],
    server: { open: true, port: 3000 },
    build: { minify: false },
});

This is what gets output w/o the Babel preset (and works as expected):

const Dashboard = component(() => {
  return /* @__PURE__ */ jsxRuntimeExports.jsxs(Stack, { spacing: 2, sx: { maxWidth: "75em" }, children: [
    /* @__PURE__ */ jsxRuntimeExports.jsx(Toolbar, {}),
    getBody()
  ] });
  function getBody() {
    switch (dashboardFormat.value) {
      case "Card":
        return unitStates().map((x) => /* @__PURE__ */ jsxRuntimeExports.jsx(UnitPanel, { state: x }, x.code));
      case "Table":
        return /* @__PURE__ */ jsxRuntimeExports.jsx(TableView, {});
    }
  }
});

[pzuraq]

Hmm, yeah that's tricky.

So, the next step I was going to do with the transform might fix this, it was to hoist callbacks that have 0 variable references to global scope. This is better for performance - each time you run that function, you're creating a new inner function.

It wouldn't solve functions that do reference variables, but we could hoist those manually to the top of the component/function as well.

[dm-pse]

That sounds like it should work.

I've read numerous times that the cost of JavaScript creating a new function is insignificant, but correct me if I'm wrong. Also, I generally only use nested functions as a way to handle details of a render or give a name to some bit of logic without the need for "prop drilling" arguments into the function, and I prefer these to be hoisted because I like the readability of the return part of the render giving me a high-level overview of what is happening (with details below). If I think perf could be an issue or things start getting complicated, I will invariably move the nested function to its own component.

[pzuraq]

So it's not just the cost of the function itself, but the cost of the added callback() wrapper which does need to check for the function in scope, compare closed over variables, etc. each time. That's how we maintain object reference stability - it's essentially one small part of what the React compiler does (but much more scoped).

[devuxer]

It sounds like you are saying that, in general, hoisted functions within a component are not as harmless for performance in React as I might have thought, but you are also saying you could update the Babel transform to at least eliminate the performance issue for hoisted functions that do not reference variables?

Also, I assume by "functions that reference variables" you are talking about functions that close over variables defined within the component (or props passed into it)?

All that said, would it solve the performance issue to move the function outside the component and pass any variables it needs into it, though still define it after the component (so it would still be hoisted but not reference any variables from the outer/global scope)? Or even leave the function within the component but pass in the variables?