Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: This plugin adds React.createElement even though it's a SolidStart project #60

Open
1 of 2 tasks
vincehi opened this issue Oct 23, 2024 · 3 comments
Open
1 of 2 tasks
Labels
bug Something isn't working

Comments

@vincehi
Copy link

vincehi commented Oct 23, 2024

What does the bug relate to?

  • Plugin's core
  • Debugger

Describe the bug

This plugin adds React.createElement even though it's a SolidStart project

import { StartClient, mount } from "@solidjs/start/client";
mount(() => /* @__PURE__ */ React.createElement(StartClient, null), document.getElementById("app"));

To Reproduce

add this plugin on the solidstart project

Vite version

0.5.1

Additional context

No response

@vincehi vincehi added the bug Something isn't working label Oct 23, 2024
@Dschungelabenteuer
Copy link
Owner

Dschungelabenteuer commented Oct 26, 2024

Hey @vincehi! Thanks for the issue, unfortunately, I'm afraid I'm not really able to help without further information about your setup. I would at least need to know how you actually set up this plugin within SolidStart's app.config.ts. If it may help, I've setup a simple example based on a basic SolidStart template!

In that example, a entry/barrel file is created in the component folder (components/index.ts). It is basically used to re-export all components. In such a scenario, you're expected to set that compoenents/index.ts as an entry inside your app.config.ts file:

import { defineConfig } from "@solidjs/start/config";
import EntryShakingPlugin from 'vite-plugin-entry-shaking';
import { resolve } from 'node:path';

export default defineConfig({
  vite: {
    plugins: [
      EntryShakingPlugin({
        targets: [
          resolve(import.meta.dirname, 'src/components'),
        ],
      }),
    ]
  }
});

In that example, you can see that the components/index.ts exports three components (Counter, Link and Unused). Both Counter and Link are rendered on the app's homepage. Unused isn't, despite being exported by components/index.ts. Here's a code representation of the above statement:

import { Counter, Link } from '~/components';

export default function App() {
  <Counter />
  <Link href="https://github.io" />
}

Now open the network tab in your browser's devtools:

  1. without this plugin, you should see a request for Unused.tsx despite it not being used in the app.
  2. with this plugin working, you should not see any request for ~/components/Unused.tsx because the plugin knows it wasn't used.

Hope it helps! If it doesn't, I'd be happy to help further but once again, I would need further information about your setup because well… yeah… React does not have anything to do with this plugin. It's supposed to be 100% framework-agnostic but there might be some kind of conflict with your very specific project.

@vincehi
Copy link
Author

vincehi commented Oct 28, 2024

Yes, excuse me,
this is my app.config.ts

import { authVite } from "@solid-mediakit/auth-plugin";
import { defineConfig } from "@solidjs/start/config";
import path, { resolve } from "node:path";
import { fileURLToPath } from "node:url";
import EntryShakingPlugin from "vite-plugin-entry-shaking";
// import Inspect from "vite-plugin-inspect";
import tsconfigPaths from "vite-tsconfig-paths";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

export default defineConfig({
	middleware: "./src/utils/middleware.ts",
	vite({ router }) {
		return {
			ssr: {
				external: ["@prisma/client"],
			},
			plugins: [
				EntryShakingPlugin({
					targets: [
						resolve(__dirname, "src/entry-client.tsx"),
					],
					maxWildcardDepth: 30,
					// diagnostics: true,
					// debug: true,
				}),

				tsconfigPaths({ root: "./" }),
				authVite({
					authOpts: {
						// the variable name of your authOptions (exported!!)
						name: "authOptions",
						//   where your authOptions is located
						dir: "@/src/utils/auth",
					},
					// where should we redirect when the user is not logged in
					redirectTo: "/login",
				}),
				// Inspect(),
			],
			server: {
				port: process.env.APP_CONTAINER_MAIN_PORT,
				...(router === "client" && {
					hmr: {
						port: process.env.APP_CONTAINER_HMR_PORT,
					},
				}),
			},
		};
	},
});

My tsconfig.json

{
  "compilerOptions": {
    "baseUrl": ".",
    "target": "ESNext",
    "module": "ESNext",
    "moduleResolution": "bundler",
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "jsx": "preserve",
    "jsxImportSource": "solid-js",
    "allowJs": true,
    "strict": true,
    "noEmit": true,
    // "sourceMap": true,
    "types": [
      "vinxi/types/client",
      "node"
    ],
    "isolatedModules": true,
    "paths": {
      "@/*": [
        "./*"
      ]
    }
  },
  "include": [
    "src/**/*",
    "server/**/*",
    "di/**/*",
    "styled-system/**/*"
  ],
  "exclude": [
    "node_modules",
    "styled-system/**/index.d.ts",
    "styled-system/**/index.mjs"
  ]
}

I can get it to work on non-JSX packages.
With this for ex:
resolve(__dirname, "node_modules/@ark-ui/solid")

@Dschungelabenteuer
Copy link
Owner

Dschungelabenteuer commented Nov 13, 2024

Hey 👋 Just wanted to let you know that I did not forget about this.

I realized my simple repro was irrelevant because I actually never imported anything from the components/index.tsx file, I've updated the repro accordingly and can since reproduce this issue. It turns out there are different problems:

TL;DR

(sorry this comment gone pretty dense)

I have an incomplete fix that should work if your entry files never actually define code that is later imported (see simple example right below). I'm not sure when I'll be able to find enough free time to further tackle this. Please let me know if you think the partial fix would help/be sufficient for your own scenario , I'm willing to deploy a first patch if this helps :)

Given the following config:

export default defineConfig({
  vite: { plugins: [EntryShakingPlugin({ targets: ["path/to/components"] })] },
});

…and the following entry file:

// path/to/components/index.tsx
import Counter from "./Counter";
import Link from "./Link";

const Div = () => <div>Div</div>;

export { Counter, Link, Div };

This would work

// consumer.ts (simply rewrites imports to `~/components/Counter.tsx` and  `~/components/Link.tsx`)
import { Counter, Link } from "~/components";

But this wouldn't:

// consumer.ts (Div is actually defined by `~/components/index.tsx` so it serves its mutated version)
import { Div } from "~/components";

The problems

1. files are excluded from transform when they should not

The function used to check whether a file should be transformed or not tries to get the requested module's extension in a very naive way and ignore possible query params (e.g. /routes/index.tsx?pick=default…). In the simple repro, the routes/index.tsx not being transformed:

  • causes the components to be resolved through the components/index.tsx
  • which is analyzed, cleaned up and served by this plugin
  • which leads to other below problem

(I did set up a branch with a fix about two weeks ago but wanted to also tackle the other problem)

2. esbuild transforms

This plugin recently added JSX support which relies on Vite's transformWithEsbuild function in order to transpile (j|t)sx entries into plain JS files so that es-module-lexer can parse them. However, it seems like using the transformWithEsbuild function with default options (jsx options are infered from the tsconfig.json - and jsx defaults to preserve in a SolidStart project) causes esbuild to add these React references.

I'm still trying to figure out a way of making it work seamlessly but unfortunately I'm not familiar with Solid and its ecosystem. I've made a bunch of tests by manually setting both jsx and jsxImportSource when esbuild runs but didn't have any chance.

I've analyzed and compared Vite's outputs both when using and not using this plugin. It seems like Solid's JSX files are actually transformed (maybe by vite-plugin-solid?) independently from this plugin's flow (which is not surprising tbh). This transformation process is what I'm looking to run some tests.

But I think the issue is even more global…

Merdouille 😬

One of the main pain points of this plugin is that it relies on es-module-lexer to extract both import and export statements, which does not support the JSX syntax. The legit need for JSX support for this plugin quickly emerged and I came up with a very naive support by simply relying on Vite's transformWithEsbuild functions since esbuild supports JSX. After all, we only need to get valid ES modules to be able to analyze and tree-shake them, don't we…

The more I think about it and about this issue, the more I think that was probably a mistake. From this plugin's perspective, infering the intent of a JSX file (or even the intent of a simple JS module) in any project is quite presomptuous. In the end, depending on (meta-)frameworks toolings, entry files could potentially require pre- or post-transforms. The main question here therefore is: when should this plugin's logic apply?

Take any scenario you want, the ideal answer would always end up being "as soon as possible" so that tree-shaking transits down descendants. Such entry files could be processed in any way by (meta-)framwork-specific tooling logic. So we'd rather apply the tree-shaking logic as close as possible from the actual source code to remain as much framework-agnostic as possible. The only thing that's actually preventing us from doing that is es-module-lexer's lack of JSX support because it requires local transpilation that may not match what your favorite framework is expecting.

In your scenario, Solid is JIT-applying its own transforms to resolved JSX files. Whenever it encounters an entry file that was processed by this plugin, it actually tries to transform the already esbuild-transpiled version instead of the expected raw JSX version. And we've seen above that this esbuild output does not necessarily match what SolidStart expects.

Basically, our issue could happen on any other JSX-based framework and a naive fix would highly depend on that framework's tooling implementation around Vite. I only see two possible reliable solutions:

  • either provide framework-specific adapters so that entry files transformed by this plugin match their framework-expected output. This would be a bit annoying imho because this plugin should work out-of-the-box no matter what the framework is for convenience, and because it would also probably depend on Vite's plugin execution order.
  • or transform JSX files (manual tree-shaking) directly from the source code without any transpilation during this plugin's static analysis. This would need this plugin to swap from es-module-lexer to another lexer that natively supports JSX. That's probably the ideal solution.

What now?

Whenever I've got enough time, I'll investigate using Boshen's oxc wasm bindings to its Rust crates or even better write my own crate on top of oxc as a drop-in replacement for es-module-lexer because:

  • their main parser support JSX
  • it shouldn't be that hard to mimic es-module-lexer's behaviour
  • despite a much wider scope (it handles a whole module while es-module-lexer specifically targets imports and exports), I do believe impacts on performance should not be very perceptible (that would require some benchmarks though).
  • Vite is set to adopt its upcoming Rolldown bundler which uses oxc for parsing. Sticking to Vite's own stack is pretty much a safe and future-proof bet (even though this plugin runs at development time while rolldown runs at build time).

Congrats if you've been brave enough to read this far 😅 In the mean time, and because I'm not sure when I'll be able to tackle this, please refer to the above Tl;DR and let me know if you'd like me to release the "incomplete" patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants