solid-refresh icon indicating copy to clipboard operation
solid-refresh copied to clipboard

use solid-refresh and solid-devtools babel plugins together?

Open tmm1 opened this issue 10 months ago • 13 comments

i'm using @solid-devtools/overlay in a project along with solid-refresh

i noticed in the overlay the props/signals/memos are missing names, which can be solved by injecting solid-devtools/babel's namePlugin into the build pipeline. using this plugin also adds graphs of the reactive relationships between components.

https://github.com/thetarnav/solid-devtools/blob/a9c23a428d5639bf138ac2f5793fc57b8eef9136/packages/main/src/babel/babel.ts#L115-L116

however when I use both the devtools and refresh plugin together, the auto names disappear. are these plugins expected to be able to work together?

i tried putting the devtools namePlugin before and after the solid-refresh one.

tmm1 avatar Jan 17 '25 05:01 tmm1

with solid-devtools only:

Image

with solid-refresh:

Image Image Image

tmm1 avatar Jan 17 '25 05:01 tmm1

Looking at the generated code, this is probably a bug on the devtools side.

I see some components are annotated:

const SidebarMenuItem = _$$component(_REGISTRY, "SidebarMenuItem", function SidebarMenuItem(props: {
}) {
  const iconClass = createMemo(() => {
...
  }, undefined, {
    name: "iconClass"
  });

whereas others are not:

const SidebarMenu = _$$component(_REGISTRY, "SidebarMenu", function SidebarMenu(props: {
}) {
  const showedPinned = createMemo(() => {
...
  });

tmm1 avatar Jan 17 '25 05:01 tmm1

Okay confirmed this is a solid-devtools bug. The workaround is:

-export function SidebarMenu(
+export { SidebarMenu };
+function SidebarMenu(

tmm1 avatar Jan 17 '25 05:01 tmm1

actually, solid-devtools works as-is with both types of components.. the issue is some kind of interaction with solid-refresh

if i put the devtools babel plugin first, nothing in the output is annotated.

if i put the devtools plugin after solid-refresh, then i get partial annotation which makes sense since devtools isn't setup for the refresh AST?

the correct configuration would seem to be devtools first so everything is annotated, then solid-refresh should be able to understand and maintain those annotations? currently it seems to remove them all?

tmm1 avatar Jan 17 '25 06:01 tmm1

well i tried a clean build with devtools plugin first and solid-refresh second and everything seems to be working fine.

EDIT: scratch that, still had my workaround in place

tmm1 avatar Jan 17 '25 06:01 tmm1

as far as I can tell, somehow when bubbleFunctionDeclaration moves a export function Component, somehow the non-exported node it adds back in is a copy from earlier in the babel pipeline.

tmm1 avatar Jan 17 '25 07:01 tmm1

~~theory from o1:~~

halluciation

Root cause explanation

From the code and transcript, here is what is actually going on:

  1. You have an exported function component, for example:

    export function SidebarMenu() {
      const [sidebarWidth, setSidebarWidth] = createSignal<number>(0, {
        name: "sidebarWidth"
      });
      …
    }
    
  2. The “solid-devtools” plugin injects or preserves the second argument object (the metadata) in the call to createSignal (e.g. “{ name: "sidebarWidth" }”).

  3. However, when “solid-refresh” wraps the function (inside transformFunctionDeclaration + wrapComponent), Babel re-creates or re-serializes the function’s AST. The key detail is that the “<number>” type parameter (a TSCallType parameter) often ends up ignored or stripped at some point (for example, if a piece of the plugin logic tries to reinterpret the call node without preserving its type parameters). As a result:

    • The AST’s call arguments effectively shift.
    • Babel ends up treating the object literal (“{ name: "sidebarWidth" }”) as if it was never there, or fabricates a single argument call (just “(0)”).

Hence the second argument with your metadata object goes missing in the final output. The simplest way to think about it is: the plugin does not fully preserve TS type parameters on function calls like createSignal, and in dropping or ignoring type information, the second argument array element winds up lost.

In other words, the bug is not in moving or re‑defining the function as a variable. It’s that the type parameter () is confusing (or being stripped by) the refresh code’s AST transforms, which causes the entire extra argument—your annotation object—to get lost when the final output is generated.

tmm1 avatar Jan 17 '25 07:01 tmm1

btw how are you setting up the plugins

lxsmnsyc avatar Jan 17 '25 08:01 lxsmnsyc

one weird thing i discovered is if i left the original component in place, then both copies would be correct.

root cause here seems to be related to how babel runs plugins. it does one pass with all of the plugins visitors running once?

the simplest fix i found so far is to move the function defintions to the bottom instead of top, which seems to ensure they get processed correctly.

const last = program.get('body').at(-1);
const [tmp] = last.insertAfter(decl);

EDIT: tht doesn't work either, just results in ReferenceError: Cannot access 'xxx' before initialization

tmm1 avatar Jan 17 '25 09:01 tmm1

btw how are you setting up the plugins

["module:solid-devtools/babel/named"],
["module:solid-refresh/babel", { bundler: "vite" }],

with patch-package and a patches/solid-devtools+0.33.0.patch

patch file
diff --git a/node_modules/solid-devtools/dist/babel/named.js b/node_modules/solid-devtools/dist/babel/named.js
new file mode 100644
index 0000000..dc3e20c
--- /dev/null
+++ b/node_modules/solid-devtools/dist/babel/named.js
@@ -0,0 +1,5 @@
+import {
+  namePlugin
+} from "../chunk-RDZMZMK7.js";
+export default function() { return namePlugin };
+
diff --git a/node_modules/solid-devtools/package.json b/node_modules/solid-devtools/package.json
index 6e3d53a..73cda29 100644
--- a/node_modules/solid-devtools/package.json
+++ b/node_modules/solid-devtools/package.json
@@ -78,6 +78,11 @@
         "default": "./dist/babel.js"
       }
     },
+    "./babel/named": {
+      "import": {
+        "default": "./dist/babel/named.js"
+      }
+    },
     "./package.json": "./package.json"
   },
   "typesVersions": {

see https://github.com/thetarnav/solid-devtools/pull/295#issuecomment-2597409241

i think probably this would work:

import { namePlugin } from 'solid-devtools/babel'

...

plugins: [
    namePlugin,
    ["module:solid-refresh/babel"]

tmm1 avatar Jan 17 '25 09:01 tmm1

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

http://thejameskyle.com/babel-plugin-ordering.html

basically if we remove the node while solid-devtools plugin is concurrently getting around to processing it, it never gets modified so the moved copy at the top of the file is the original AST.

https://github.com/babel/babel/issues/15265#issuecomment-1347327694

https://github.com/babel/babel/issues/11147#issuecomment-902714234

tmm1 avatar Jan 17 '25 09:01 tmm1

http://thejameskyle.com/babel-plugin-ordering.html

this post describes the underlying issue well. both plugins use a Program visitor so ordering is problematic.

i tried to wrangle babel to make this work, but in the end i had to do it manually.

--- a/node_modules/solid-refresh/dist/babel.mjs
+++ b/node_modules/solid-refresh/dist/babel.mjs
@@ -1,6 +1,7 @@
 import * as t from '@babel/types';
 import path from 'path';
 import _generator from '@babel/generator';
+import { namePlugin } from 'solid-devtools/babel';

 // This is just a Pascal heuristic
 // we only assume a function is a component
@@ -1008,6 +1009,9 @@ function solidRefreshPlugin() {
                 if (setupProgram(state, programPath, context.file.ast.comments)) {
                     return;
                 }
+                const devtools = namePlugin.visitor
+                programPath._call(devtools.Program.enter)
+                programPath.traverse(devtools);
                 programPath.traverse({
                     FunctionDeclaration(path) {
                         bubbleFunctionDeclaration(programPath, path);

tmm1 avatar Jan 18 '25 00:01 tmm1

that's not exactly reliable, but I can't blame you for that.

lxsmnsyc avatar Jan 18 '25 12:01 lxsmnsyc