Skip to content

svelte-preprocess generate invalid CSS #191

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

Closed
wighawag opened this issue Jun 27, 2020 · 7 comments · Fixed by #193
Closed

svelte-preprocess generate invalid CSS #191

wighawag opened this issue Jun 27, 2020 · 7 comments · Fixed by #193
Assignees
Labels
bug Something isn't working next-major will be fixed in the next major

Comments

@wighawag
Copy link

Describe the bug
svelte-preprocess generate invalid CSS when a global style include valid css using backslash

see sveltejs/svelte#5078

Logs

from https://github.com/wighawag/svelte-css-parser-error/

$ rollup -c

src/main.js → public/build/bundle.js...
[!] (plugin svelte) ParseError: RightParenthesis is expected
src\App.svelte
178:    );
179:    --color-icon: var(--color-positive-normal-muted);
180:  }
       ^
181:  </style>
182:
ParseError: RightParenthesis is expected
    at error (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\utils\error.ts:25:16)
    at Parser$1.error (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\parse\index.ts:93:3)
    at Object.read_style [as read] (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\parse\read\style.ts:21:11)
    at tag (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\parse\state\tag.ts:190:27)
    at new Parser$1 (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\parse\index.ts:45:12)
    at parse (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\parse\index.ts:208:17)
    at compile (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\svelte\src\compiler\compile\index.ts:79:14)
    at C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\rollup-plugin-svelte\index.js:252:22
    at ModuleLoader.addModuleSource (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\rollup\dist\shared\rollup.js:17749:30)
    at ModuleLoader.fetchModule (C:\dev\projects\wighawag\svelte-css-parser-test-2\node_modules\rollup\dist\shared\rollup.js:17803:9)

To Reproduce

git clone https://github.com/wighawag/svelte-css-parser-error/
cd svelte-css-parser-error
yarn && yarn build

Expected behavior
should build succesfully with valid css in <style global > </style> tags

Stacktraces

see log above

Information about your Svelte project:

  • Your operating system: Windows 10

  • Svelte version : 3.23.2

  • Whether your project uses Webpack or Rollup : Rollup

Severity
It block me from using css library like a17t so pretty severe in my opinion

@kaisermann
Copy link
Member

kaisermann commented Jun 27, 2020

Hey @wighawag 👋 Thanks for the report and the repro 😁 This is happening due to the change introduced in #168. The regexp we use to split a selector into multiple parts should ignore escaped ~ characters. Unfortunately, I don't have the time right now to find a fix or an alternative lightweight way of splitting the selector, but I'll come up with a possible solution soon. In the mean time, any help is appreciated 👀

@kaisermann kaisermann self-assigned this Jun 27, 2020
@kaisermann kaisermann added bug Something isn't working help wanted Extra attention is needed labels Jun 27, 2020
kaisermann added a commit that referenced this issue Jun 30, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
@kaisermann
Copy link
Member

This would be easily fixed with something like postcss-selector-parser, but I'm really trying to keep this library as lightweight as possible and adding 49.9kb to it is definitely undesired. I found a way of fixing it by using lookbehind assertions (#193), but this change can be a breaking one for some people since lookbehind assertions are only available in Node 9.11.2+. I'll keep the PR open for now and decide if the risk of merging with v3 is worth taking or if I should merge it with the v4 branch (#171)

@kaisermann kaisermann removed the help wanted Extra attention is needed label Jun 30, 2020
kaisermann added a commit that referenced this issue Jul 1, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
kaisermann added a commit that referenced this issue Jul 1, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
kaisermann added a commit that referenced this issue Jul 1, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
kaisermann added a commit that referenced this issue Jul 1, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
@kaisermann kaisermann added the next-major will be fixed in the next major label Jul 1, 2020
@kaisermann
Copy link
Member

As I don't want to ruin the experience for those using node v8.x or lower, this one will be fixed when v4 lands 😁

@kaisermann
Copy link
Member

Just released v4-alpha.1 with the fix! You can already try it with npm i -D svelte-preprocess@next. The changelog can be found here (https://github.com/sveltejs/svelte-preprocess/blob/v4/CHANGELOG.md#400-alpha1-2020-07-05).

kaisermann added a commit that referenced this issue Jul 5, 2020
BREAKING CHANGE: 🧨 Uses Lookbehind assertions, so Node 9.11.2+ is needed

✅ Closes: Closes #191
@matheusbenedet
Copy link

Hello, i'm having the same issue:

image

"svelte-preprocess": "^4.0.0-alpha.2",

@kaisermann
Copy link
Member

kaisermann commented May 5, 2021

@vendaagil Can you share a repro?

@fernandolguevara
Copy link

fernandolguevara commented Jun 21, 2021

same error with sveltekit
image

@kaisermann

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

Successfully merging a pull request may close this issue.

4 participants