preactjs / preset-vite

Preset for using Preact with the vite bundler
https://npm.im/@preact/preset-vite
MIT License
262 stars 26 forks source link

Preact+Vite JS transforms cause code to break #119

Closed trusktr closed 6 months ago

trusktr commented 6 months ago

How do we disable JS transforms? I have code that works fine in plain JS (no build). When I add Preact+Vite, the code is transformed into code for old browsers, and it breaks in strange ways (private fields, etc, are compiled to something for old browsers).

It looks like Preact's Vite plugin hard codes transforms, and it is not configurable:

https://github.com/preactjs/preset-vite/blob/aef0301bcf1f02ebc44433da331469d2ac70d99a/src/index.ts#L178-L185

So it is impossible to disable private fields, etc, from being compiled.

All I want is the JSX transform, nothing else.

rschristian commented 6 months ago

All I want is the JSX transform, nothing else.

Simplest solution for now would be to add the following to your Vite config and forgo the JSX plugin we ship:

export default defineConfig({
    build: {
        esbuild: {
            jsx: "automatic",
            jsxImportSource: "preact",
        }
    }
});

Edit: That being said, to my understanding, parser plugins shouldn't transform anything. A reproduction would be helpful.

trusktr commented 6 months ago

I'm not sure what is exactly causing the issue, but I see stuff like __privateGet and __privateSet everywhere instead of private fields, etc. Basically the code is far-removed from what I wrote, and something is breaking it.

I tried your last tip to use jsx:automatic in the app I made with npm init preact after removing the preact preset, but I get

Uncaught ReferenceError: React is not defined

and the compiled code has React.createElement in it. Do I need a newer Vite than the one create-preact-app has?

EDIT: I just tried

        esbuild: {
            jsxFactory: 'h',
            jsxImportSource: 'preact',
        },

but it has the same React issue (I dislike React being default everwhere sooooo much).

rschristian commented 6 months ago

I'm not sure what is exactly causing the issue, but I see stuff like privateGet and privateSet everywhere instead of private fields, etc.

Have you altered Vite's targets? Vite's default targets should be: 'es2020', 'edge88', 'firefox78', 'chrome87', 'safari14'. Docs.

I tried your last tip to use jsx:automatic in the app I made with npm init preact after removing the preact preset, but I get

Whoops, my bad. This is what you want:

export default defineConfig({
    esbuild: {
        jsx: "automatic",
        jsxImportSource: "preact",
    }
});

Edit: That being said, this should get you what you want:

export default defineConfig({
    build: {
        target: 'esnext'
    },
    plugins: [preact()]
});

From a few quick tests, we're not transpiling anything here beyond JSX by default. Vite's default targets will do a bit of work though, so adjust them as you need/want. Should be able to keep on using the Preact plugin w/out issue.

trusktr commented 6 months ago

Whoops, my bad. This is what you want:

Ah, thanks! I do get the same error (without Preact Vite preset). I suppose this issue can be closed because it isn't due to the Preact Vite preset.

Have you altered Vite's targets?

Here's vite.config.js:

import {defineConfig} from 'vite'

export default defineConfig({
    build: {
        target: 'esnext',
    },
    esbuild: {
        jsx: 'automatic',
        jsxImportSource: 'preact',
    },
})

Here's jsconfig.json:

{
    "compilerOptions": {
        "target": "ES2022",
        "module": "ESNext",
        "moduleResolution": "bundler",
        "noEmit": true,
        "allowJs": true,
        "checkJs": false,

        /* Preact Config */
        "skipLibCheck": true
    },
    "include": ["node_modules/vite/client.d.ts", "**/*"]
}

I also tried build.target=esnext but I still get compiled code like this:

        __privateSet(this, _task, Motor.addRenderTask((_t, dt) => {
          const dx2 = __privateGet(this, _targetX) - this._x;
          const dy2 = __privateGet(this, _targetY) - this._y;
          const fpsRatio = dt / 16.6666;

I double checked, and the source code from the library I'm consuming before building the app with vite has code like this:

            this.#task = Motor.addRenderTask((_t, dt) => {
                const dx = this.#targetX - this._x;
                const dy = this.#targetY - this._y;
                const fpsRatio = dt / 16.6666;
rschristian commented 6 months ago

Ah, thanks! I do get the same error (without Preact Vite preset).

The React is not defined error? Strange, are you using a React node module? If so, you will need to set up aliasing too, which is another fairly simple copy/paste.

I also tried build.target=es2023 but I still get compiled code like this:

Is this after vite build, or are you checking the output in dev? If dev, you'll need to use --force to get Vite to re-precompile the module -- it'll do it once and then never again, leaving you with outdated results. If after a build, that's odd though. This is my test:

src/index.jsx


class Foo {
#bar = 'baz';
getBar() {
return this.#bar;
}   
}

export function App() { const foo = new Foo(); console.log(foo.getBar()); return (...) }


> dist/assets/index-<hash>.js (prettified)
```js
class he{
    #e="baz";
    getBar(){
        return this.#e
    }
}

Here's a live demo: https://stackblitz.com/edit/vitejs-vite-a3emp2?file=dist%2Fassets%2Findex-DOTlD5Sk.js

trusktr commented 6 months ago

The React is not defined error?

Sorry no, I mean the same build output causing the same error with my JS code (Preact JSX is fine now (like it was with the preset I imagine)).

Is this after vite build, or are you checking the output in dev?

I was running vite, then looking at the code in browser devtools. I tried vite --force but no luck.

I managed to reproduce it here based on your blitz:

https://stackblitz.com/edit/vitejs-vite-mykqek?file=package.json,src%2Fmain.jsx,vite.config.js,src%2Fstyle.css,public%2Fvite.svg

When you try scrolling in the 3D scene (with wheel or two fingers) to zoom in or out, the error happens in console. If you click on this part of the stack:

Screenshot 2024-05-14 at 2 52 45 PM

You'll see the transpiled code with __privateGet like the one I pasted above.

In the stackblitz, if you run cat node_modules/lume/dist/interaction/ScrollFling.js you can see the original source has original private fields.

Any idea what's doing that transpile?


(closing, not Preact preset issue)

rschristian commented 6 months ago

So this feels like a foot cannon to me, but apparently Vite lets you configure prebundling targets separately? I have no idea why anyone would want this.

export default defineConfig({
    build: {
        target: 'esnext'
    },
    optimizeDeps: {
        esbuildOptions: {
             target: 'esnext'
        }
    },
    plugins: [preact()]
});

This should fix your class fields in dev (make sure you use --force to get Vite to recompile), however, there seems to still be some issues.

And no worries, sorry this isn't working out for you.

trusktr commented 6 months ago

Awesome, that does prevent all the extra build stuff, but the same error happens. So that's not the issue.

Blitz: https://stackblitz.com/edit/vitejs-vite-jqjrsr?file=main.jsx,vite.config.js

Is Preact modifying the environment in any way, f.e. modifying or intercepting DOM events, or patching globals?

So far I've used Lume with plain HTML, plain JS, Vue, Svelte, React, Lit, and Lume Element without this issue. This is my first Preact attempt, and the issue is popping up.

What I'm seeing is JS properties on the elements have string values on them from attributes set in Preact. But the properties should handle strings fine (they do in the other frameworks I've tried, not sure why they wouldn't work in this Preact setup).

rschristian commented 6 months ago

Is Preact modifying the environment in any way, f.e. modifying or intercepting DOM events, or patching globals?

Hey we're Preact, not React!

What I'm seeing is JS properties on the elements have string values on them from attributes set in Preact.

Can you be a bit more specific? You can't specifically set attrs or properties w/ Preact (or React, for that matter). Which gets set is an implementation detail really.

trusktr commented 6 months ago

Hey we're Preact, not React!

Haha, noted.

Can you be a bit more specific?

The value from distance="301" (in that blitz example's JSX) somehow ends up as a string on the element's .distance JS property. Normally Lume elements should be able to accept string values and handle them (that's how all the HTML attributes work, as they're mapped to JS properties).

Not sure what's going on, maybe something regressed in Lume. I'll keep investigating. Thanks for the help!

rschristian commented 6 months ago

(that's how all the HTML attributes work, as they're mapped to JS properties).

Just to be clear, what are you expecting here? That the property gets set, or the attribute?

Preact will only set the property, not the attribute, and they are different things (el.getAttribute('distance') vs el.distance). Many (most?) properties do not reflect to attributes.

trusktr commented 6 months ago

That the property gets set, or the attribute?

Either is fine: an attribute string is mapped to a property by the element's attributeChangedCallback, and the property handles the string and (should) coerce it to a non-string, so if Preact sends a string to the property, that should be fine. But for some reason that string handling is not happening in Lume. I'm thinking there's a possibility this version of Lume broke (JS properties no longer handling strings).

rschristian commented 6 months ago

Fair enough, just wanted to double check as some users do expect one or the other (or both) to be set.

It's certainly possible for this to be a Preact bug but we'd need a minimal reproduction as Lume seems to be quite big. Happy to help where I can if you need me though, feel free to ping. Good luck!

trusktr commented 6 months ago

some users do expect one or the other (or both) to be set.

Yeah, although no a problem in my case, I do think this a valid thing for users to want. I proposed to add a new test to custom-elements-everywhere to test that frameworks allow users to choose.

Happy to help where I can if you need me though, feel free to ping. Good luck!

Thank you! Will keep digging. 👷 ⛏️

rschristian commented 6 months ago

I do think this a valid thing for users to want.

I'm certainly all for it (or a variation of it) and I'd guess some other Preact members are too. JSX is a slow beast to change, sadly, it'll be hard to land a major syntax change. Would simplify a lot of our code though.

trusktr commented 6 months ago

Pota's JSX supports attr:foo="", prop:foo="", and bool:foo="", which has the same parity with Lit's foo=, .foo=, and ?foo=, allowing users to have total control over setting an attribute, setting a prop, or toggling a boolean attribute (adding/removing). The attr: syntax is valid in JSX, and Preact's h would just need to handle that string.

It turns out the issue is in Lume element: a while ago (more than year!) the JS setters stopped handling strings (React 19 will cause the issue to become apparent too). In other template systems like Solid JSX, foo="" sets an attribute while foo={} sets the JS property on custom elements (and in that case type checking keeps me sending in the correct non-string types). Not sure how I've been getting lucky in Svelte and Vue templates, but likely something similar with foo="" setting attributes, or else just pure luck I didn't use some specific attributes (because some other attributes work fine if they are strings even when they shouldn't be).

rschristian commented 6 months ago

The attr: syntax is valid in JSX, and Preact's h would just need to handle that string.

Indeed, but Babel and some other transpilers will error upon that by default (if configurable at all -- not all are).

For Preact, we basically do if (prop in domElement) domElement[prop] = value; else domElement.setAttr(prop, value) with a few exceptions, in case that's useful to know.