search

sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.64k stars 4.22k forks source link

Svelte 5: Forward compatibility break caused by #11954 #12518

Closed qupig closed 3 months ago

qupig commented 3 months ago

Describe the bug

[!NOTE] I know there are "hundred" ways to get around and fix the issue or improve the writeup, so PLEASE don't tell me how I should write code.

But please take into account that I spend hours locating the issue due to the commonness of the original class name, and that I've already fixed the code in my own project. I spent extra time making a minimal reproduction as well as reporting the issue.

The issue is reported because it causes forward compatibility breakage and others may encounter the same error.

The issue has been located and confirmed to be caused by #11954.

So the versions with the issue are: svelte@5.0.0-next.152 ~ svelte@5.0.0-next.192

In earlier versions as well as svelte@4, the same code does not cause the issue.

The issue only occurs during vite build and not dev.

Reproduction

StackBlitz-Svelte-5.0.0-next.192

<script>
    let color;
</script>

<div class="abc {color || 'red'}"></div>

Logs

vite v5.3.4 building for production...
✓ 80 modules transformed.
x Build failed in 580ms
error during build:
[vite:esbuild-transpile] Transform failed with 1 error:
assets/index-!~{001}~.js:1434:51: ERROR: Expected ")" but found "}"

Expected ")" but found "}"
1432|   var div = root();
1433|  
1434|   template_effect(() => set_class(div, `abc ${('red'}`));
    |                                                     ^
1435|   append($$anchor, div);
1436|  }

    at failureErrorWithLog (/home/projects/vitejs-vite-eaupzs/node_modules/esbuild/lib/main.js:1462:15)
    at start/< (/home/projects/vitejs-vite-eaupzs/node_modules/esbuild/lib/main.js:745:50)
    at sendRequest/responseCallbacks[id] (/home/projects/vitejs-vite-eaupzs/node_modules/esbuild/lib/main.js:612:17)
    at handleIncomingPacket (/home/projects/vitejs-vite-eaupzs/node_modules/esbuild/lib/main.js:667:20)
    at readFromStdout (/home/projects/vitejs-vite-eaupzs/node_modules/esbuild/lib/main.js:590:27)
    at emit (node:events:30:10899)
    at addChunk (node:internal/streams/readable:225:3685)
    at readableAddChunk (node:internal/streams/readable:225:3393)
    at internal/streams/readable/Readable.prototype.push (node:internal/streams/readable:225:4971)
    at onStreamRead (node:internal/stream_base_commons:211:2596)
    at _0x5a72d5/< (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:515096)
    at _0x14e07c/< (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:517084)
    at _0x19b239/< (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:195382)
    at _0x3eacb0 (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:195487)
    at _0x14e07c (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:516979)
    at _0x1fa6c6 (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:513649)
    at _0x8b8b7a/< (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:517356)
    at _0x8b8b7a (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:517334)
    at _0x2d466d (https://vitejsviteeaupzs-c4v3.w-corp-staticblitz.com/blitz.41692973.js:40:516307)

System Info

svelte@5.0.0-next.152

Severity

annoyance

paoloricciuti commented 3 months ago

Mmm this is weird...the code compiled from svelte looks correct to me

import * as $ from "svelte/internal/client";

var root = $.template(`<div></div>`);

export default function App($$anchor) {
    let color;
    var div = root();

    $.set_class(div, `abc ${(color || 'red') ?? ""}`);
    $.append($$anchor, div);
}

I wonder if it's an upstream problem about how the code gets tree shaken. How can you pinpoint the PR? Just because it was the only change in that version?

qupig commented 3 months ago

I wonder if it's an upstream problem about how the code gets tree shaken. How can you pinpoint the PR? Just because it was the only change in that version?

@paoloricciuti Why don't you go and try it yourself? See if I'm right?

paoloricciuti commented 3 months ago

How can you pinpoint the PR? Just because it was the only change in that version?

@paoloricciuti Why don't you go and try it yourself? See if I'm right?

What should I try? I'm asking what process did you use to pinpoint that PR because that could help better understand what the actual issue is.

qupig commented 3 months ago

What should I try? I'm asking what process did you use to pinpoint that PR because that could help better understand what the actual issue is.

@paoloricciuti

I don't have to respond to your questions when you don't say "please" and question my work.

The issue and reproduction is there for you to check and verify on your own, or you can ignore and close the issue like other.

paoloricciuti commented 3 months ago

What should I try? I'm asking what process did you use to pinpoint that PR because that could help better understand what the actual issue is.

@paoloricciuti

I don't have to respond to your questions when you don't say "please" and question my work.

The issue and reproduction is there for you to check and verify on your own, or you can ignore and close the issue like other.

Where did I question your work? I really don't get why you are always so oppositive against people trying to help you. To be clear I see the issue as well and in a sveltekit project too so there's definitely a problem here. However the compiled code from svelte seems correct and different from what's shown in the frame of the compilation error.

So my thinking is that this is an issue in plugin-vite-svelte or maybe even higher up in esbuild-transpile. But I could be wrong so I wanted more information about how you pinpointed the PR because if you are sure that that pr introduced the bug maybe you have more info about the problem.

qupig commented 3 months ago

Where did I question your work?

How can you pinpoint the PR? Just because it was the only change in that version?

@paoloricciuti The following quotes GPT’s analysis:

Yes, the statement does contain a tone of doubt. The phrase "Just because it was the only change in that version?" questions the validity of pinpointing the PR based solely on it being the only change in that version.

I really don't get why you are always so oppositive against people trying to help you.

And I really don't need your help, I wrote very clearly that I have solved my problem and that I am providing feedback to help the project.

And if you really need help finding the answer to that question, here is an ECE video to help you.

paoloricciuti commented 3 months ago

Where did I question your work?

How can you pinpoint the PR? Just because it was the only change in that version?

@paoloricciuti The following quotes GPT’s analysis:

Yes, the statement does contain a tone of doubt. The phrase "Just because it was the only change in that version?" questions the validity of pinpointing the PR based solely on it being the only change in that version.

Well if GPT says so lol

Joking aside it was an attempt to allow you to just answer "yes" if that was the case. Since it's not the case the question is still the same and it would help me understand the issue. So if we can please avoid being defensive my question was genuine. How can you pinpoint the PR? I'm not questioning your work, just trying to understand if you have additional information that can help me understand where the issue originate from.

I want to avoid start researching eventual problems in the vite plugin if you already know that the issue is in that pr.

paoloricciuti commented 3 months ago

Where did I question your work?

How can you pinpoint the PR? Just because it was the only change in that version?

@paoloricciuti The following quotes GPT’s analysis:

Yes, the statement does contain a tone of doubt. The phrase "Just because it was the only change in that version?" questions the validity of pinpointing the PR based solely on it being the only change in that version.

I really don't get why you are always so oppositive against people trying to help you.

And I really don't need your help, I wrote very clearly that I have solved my problem and that I am providing feedback to help the project.

And if you really need help finding the answer to that question, here is an ECE video to help you.

Ok I'm done with you. I'm here for the same reason, to help the project and what I get back from you from my free time that I spend helping the project is just bitterness because you think I'm questioning your work while I was just trying to understand what you did.

You are heavily biased against me evidently so I will not interact with you anymore.

Never blocked anyone on any platform but I will make an exception this time. See you.

qupig commented 3 months ago

Well if GPT says so lol

Joking aside it was an attempt to allow you to just answer "yes" if that was the case. Since it's not the case the question is still the same and it would help me understand the issue. So if we can please avoid being defensive my question was genuine. How can you pinpoint the PR? I'm not questioning your work, just trying to understand if you have additional information that can help me understand where the issue originate from.

I want to avoid start researching eventual problems in the vite plugin if you already know that the issue is in that pr.

I'm not joking, what do you think I wrote the big NOTE at the beginning for?

And I don't even understand where the answer you designed comes from?

How do you think that was the only change in the version? Have you even checked?

https://github.com/sveltejs/svelte/compare/svelte%405.0.0-next.151...svelte%405.0.0-next.152

gtm-nayan commented 3 months ago

Bug lies in Rollup, See https://rollupjs.org/repl/?version=4.19.0&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMGRlZmF1bHQlMjBmdW5jdGlvbiUyMEFwcCglMjQlMjRhbmNob3IpJTIwJTdCJTVDbiU1Q3RsZXQlMjBjb2xvciUzQiU1Q24lNUN0c2V0X2NsYXNzKCU2MGFiYyUyMCUyNCU3Qihjb2xvciUyMCU3QyU3QyUyMCdyZWQnKSUyMCUzRiUzRiUyMCU1QyUyMiU1QyUyMiU3RCU2MCklM0IlNUNuJTdEJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCUyQyU3QiUyMmNvZGUlMjIlM0ElMjIlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlMkMlMjJuYW1lJTIyJTNBJTIybWF0aHMuanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0F0cnVlJTdEJTdE

qupig commented 3 months ago

It looks like the problem may be in Rollup, but to be clear, this is still related to the changes in #11954, and before this PR, the issue would not be reproduced even in the same Rollup version:

StackBlitz-Svelte-5.0.0-next.151

Logs

~/projects/vitejs-vite-g26iia
❯ npm run dev

> vite-svelte-starter@0.0.0 dev
> vite build

15:46:51 [vite-plugin-svelte] You are using Svelte 5.0.0-next.151. Svelte 5 support is experimental, breaking changes can occur in any release until this notice is removed.
work in progress:
 - svelte-inspector is disabled until dev mode implements node to code mapping

vite v5.3.4 building for production...
✓ 77 modules transformed.
dist/index.html                 0.46 kB │ gzip: 0.30 kB
dist/assets/index-DPDjIYFo.css  1.00 kB │ gzip: 0.54 kB
dist/assets/index-CwAZz3dl.js   8.41 kB │ gzip: 3.85 kB
✓ built in 606ms

~/projects/vitejs-vite-g26iia 3s

❯ npm ls -a
npm ERR! code ELSPROBLEMS
npm ERR! invalid: svelte@5.0.0-next.151 /home/projects/vitejs-vite-g26iia/node_modules/svelte
vite-svelte-starter@0.0.0 /home/projects/vitejs-vite-g26iia
+-- @sveltejs/vite-plugin-svelte@3.1.1
| +-- @sveltejs/vite-plugin-svelte-inspector@2.1.0
| | +-- @sveltejs/vite-plugin-svelte@3.1.1 deduped
| | +-- debug@4.3.5 deduped
| | +-- svelte@5.0.0-next.151 deduped
| | `-- vite@5.3.4 deduped
| +-- debug@4.3.5
| | `-- ms@2.1.2
| +-- deepmerge@4.3.1
| +-- kleur@4.1.5
| +-- magic-string@0.30.10
| | `-- @jridgewell/sourcemap-codec@1.5.0 deduped
| +-- svelte-hmr@0.16.0
| | `-- svelte@5.0.0-next.151 deduped invalid: "^3.19.0 || ^4.0.0" from node_modules/@sveltejs/vite-plugin-svelte/node_modules/svelte-hmr
| +-- svelte@5.0.0-next.151 deduped
| +-- vite@5.3.4 deduped
| `-- vitefu@0.2.5
|   `-- vite@5.3.4 deduped
+-- svelte@5.0.0-next.151
| +-- @ampproject/remapping@2.3.0
| | +-- @jridgewell/gen-mapping@0.3.5
| | | +-- @jridgewell/set-array@1.2.1
| | | +-- @jridgewell/sourcemap-codec@1.5.0 deduped
| | | `-- @jridgewell/trace-mapping@0.3.25 deduped
| | `-- @jridgewell/trace-mapping@0.3.25
| |   +-- @jridgewell/resolve-uri@3.1.2
| |   `-- @jridgewell/sourcemap-codec@1.5.0 deduped
| +-- @jridgewell/sourcemap-codec@1.5.0
| +-- @types/estree@1.0.5
| +-- acorn-typescript@1.4.13
| | `-- acorn@8.12.1 deduped
| +-- acorn@8.12.1
| +-- aria-query@5.3.0
| | `-- dequal@2.0.3
| +-- axobject-query@4.1.0
| +-- esm-env@1.0.0
| +-- esrap@1.2.2
| | +-- @jridgewell/sourcemap-codec@1.5.0 deduped
| | `-- @types/estree@1.0.5 deduped
| +-- is-reference@3.0.2
| | `-- @types/estree@1.0.5 deduped
| +-- locate-character@3.0.0
| +-- magic-string@0.30.10 deduped
| `-- zimmerframe@1.1.2
`-- vite@5.3.4
  +-- UNMET OPTIONAL DEPENDENCY @types/node@^18.0.0 || >=20.0.0
  +-- esbuild@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/aix-ppc64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/android-arm@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/android-arm64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/android-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/darwin-arm64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/darwin-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/freebsd-arm64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/freebsd-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-arm@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-arm64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-ia32@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-loong64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-mips64el@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-ppc64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-riscv64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/linux-s390x@0.21.5
  | +-- @esbuild/linux-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/netbsd-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/openbsd-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/sunos-x64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/win32-arm64@0.21.5
  | +-- UNMET OPTIONAL DEPENDENCY @esbuild/win32-ia32@0.21.5
  | `-- UNMET OPTIONAL DEPENDENCY @esbuild/win32-x64@0.21.5
  +-- UNMET OPTIONAL DEPENDENCY fsevents@~2.3.3
  +-- UNMET OPTIONAL DEPENDENCY less@*
  +-- UNMET OPTIONAL DEPENDENCY lightningcss@^1.21.0
  +-- postcss@8.4.39
  | +-- nanoid@3.3.7
  | +-- picocolors@1.0.1
  | `-- source-map-js@1.2.0
  +-- rollup@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-android-arm-eabi@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-android-arm64@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-darwin-arm64@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-darwin-x64@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-arm-gnueabihf@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-arm-musleabihf@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-arm64-gnu@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-arm64-musl@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-powerpc64le-gnu@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-riscv64-gnu@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-linux-s390x-gnu@4.19.0
  | +-- @rollup/rollup-linux-x64-gnu@4.19.0
  | +-- @rollup/rollup-linux-x64-musl@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-win32-arm64-msvc@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-win32-ia32-msvc@4.19.0
  | +-- UNMET OPTIONAL DEPENDENCY @rollup/rollup-win32-x64-msvc@4.19.0
  | +-- @types/estree@1.0.5 deduped
  | `-- UNMET OPTIONAL DEPENDENCY fsevents@~2.3.2
  +-- UNMET OPTIONAL DEPENDENCY sass@*
  +-- UNMET OPTIONAL DEPENDENCY stylus@*
  +-- UNMET OPTIONAL DEPENDENCY sugarss@*
  `-- UNMET OPTIONAL DEPENDENCY terser@^5.4.0
qupig commented 3 months ago

Can confirm that the error did occur in Rollup, thanks to @gtm-nayan for investigating and reproducing it.

Due to the addition of ?? '' in #11954, which triggered this particular bug present in Rollup.

If remove it rollup treeshake works fine and that should be the complete appearance of the issue.

Rollup-REPL

I have reported the issue to rollup and all credit goes to @gtm-nayan, thank you!

gterras commented 3 months ago

Why don't you go and try it yourself? See if I'm right?

I don't have to respond to your questions when you don't say "please" and question my work.

And I really don't need your help,

I'm not joking, what do you think I wrote the big NOTE at the beginning for?

Sorry for off-topic but honestly this behavior should be noted. This is ridiculously aggressive for no reason. Accepting collaborators to get talked to like that is detrimental to the project and should not be accepted.

paoloricciuti commented 3 months ago

Why don't you go and try it yourself? See if I'm right?

I don't have to respond to your questions when you don't say "please" and question my work.

And I really don't need your help,

I'm not joking, what do you think I wrote the big NOTE at the beginning for?

Sorry for off-topic but honestly this behavior should be noted. This is ridiculously aggressive for no reason. Accepting collaborators to get talked to like that is detrimental to the project and should not be accepted.

It's fine, it's not maintainers duty to intervene in discussions and probably some sentence I wrote could've been misinterpreted. But I'll definitely stop interacting with him.

Rich-Harris commented 3 months ago

Sorry for off-topic but honestly this behavior should be noted

It has been. If @qupig behaves like this in future they will no longer be welcome here.

qupig commented 3 months ago

Sorry for off-topic but honestly this behavior should be noted

It has been. If @qupig behaves like this in future they will no longer be welcome here.

Then I won't be back. Respect is mutual and every developer and contributor deserves the same.

I appreciate you creating this project, but I'm sorry you have to maintain this community in this way.

@Rich-Harris Thanks and take care.

brunnerh commented 3 months ago

Nowhere have you been disrespected - you took it upon yourself to misinterpret every request for additional details to help resolve issues as a personal attack on you.

Rich-Harris commented 3 months ago

I have blocked @qupig from the organisation.

paoloricciuti commented 3 months ago

I have blocked @qupig from the organisation.

Sorry for causing such a hassle. And thanks everyone for the kind words! 🙏🏻