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

Linaria 6: broken :global selector (regression) #1388

Closed
PierreGUI opened this issue Dec 19, 2023 · 13 comments · Fixed by #1391
Closed

Linaria 6: broken :global selector (regression) #1388

PierreGUI opened this issue Dec 19, 2023 · 13 comments · Fixed by #1391
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: webpack 📦 Issue is related to webpack bundler needs: complete repro 🖥️ Issue need to have complete repro provided

Comments

@PierreGUI
Copy link

PierreGUI commented Dec 19, 2023

Environment

Linaria version: 6
Bundler: Webpack 5

Description

Original question: "Linaria 6: space between classname and pseudo element selector (regression)", rephrased based on answers, see below.
This code generates a class name with an unwanted space: .dhnsy8v ::after {

const Decorated = styled.h1`
  ::after {
     ...
  }
`;

The expected behaviour is no space between classname and ::after

Reproducible Demo

https://stackblitz.com/edit/linaria-bug-y4gxid?file=src%2FTitle.tsx

  • It works with Linaria 5
  • It works when written &::after { ... }
@PierreGUI PierreGUI added bug report 🦗 Issue is probably a bug, but it needs to be checked needs: complete repro 🖥️ Issue need to have complete repro provided needs: triage 🏷 Issue needs to be checked and prioritized labels Dec 19, 2023
@github-actions github-actions bot added bundler: webpack 📦 Issue is related to webpack bundler and removed needs: triage 🏷 Issue needs to be checked and prioritized labels Dec 19, 2023
@PierreGUI
Copy link
Author

PierreGUI commented Dec 19, 2023

PS: same problem with :hover -> .dhnsy8v :hover {
(and :first-of-type, etc...)

@PierreGUI
Copy link
Author

BTW this is the reason why Linaria's own website looks a bit off https://linaria.dev

image

@agriffis
Copy link
Contributor

This isn't really a regression, rather it's a breaking change in v6. It's because of stylis v4, see #1373

The new behavior matches SCSS and other preprocessors. Linaria was previously an outlier, and anyone migrating had to change their nested selectors.

@alekseykulikov
Copy link

I confirm that the documented :global() selector doesn't work in v6 (https://linaria.dev is an excellent proof). Do you have any suggestions on how to define global styles with v6?

@agriffis
Copy link
Contributor

I confirm that the documented :global() selector doesn't work in v6 (https://linaria.dev is an excellent proof). Do you have any suggestions on how to define global styles with v6?

I'm finding the same thing. It seems Linaria or wyw-in-js needs an adjustment to enable global styles with Stylis v4, but I haven't yet traced the code to understand how it is intended to work.

@agriffis
Copy link
Contributor

agriffis commented Dec 27, 2023

So it appears that we need the namespace middleware to support :global() rules.

When I add it to wyw like this:

diff --git a/packages/transform/src/transform/generators/extract.ts b/packages/transform/src/transform/generators/extract.ts
index 215d27b..66aec9f 100644
--- a/packages/transform/src/transform/generators/extract.ts
+++ b/packages/transform/src/transform/generators/extract.ts
@@ -2,7 +2,14 @@ import path from 'path';
 
 import type { Mapping } from 'source-map';
 import { SourceMapGenerator } from 'source-map';
-import { compile, serialize, stringify, middleware, prefixer } from 'stylis';
+import {
+  compile,
+  serialize,
+  stringify,
+  middleware,
+  namespace,
+  prefixer,
+} from 'stylis';
 
 import type { Replacements, Rules } from '@wyw-in-js/shared';
 
@@ -50,6 +57,7 @@ function createStylisPreprocessor(options: Options) {
             );
           }
         },
+        namespace,
         prefixer,
         stringify,
       ])

then :global(html) {...} works but :global() {html {...}} doesn't. It's progress but still broken.

Unfortunately the code for the namespace middleware is questionable at best. It might be faster to come up with a new replacement middleware than to debug it.

@PierreGUI PierreGUI changed the title Linaria 6: space between classname and pseudo element selector (regression) Linaria 6: broken :global selector (regression) Jan 2, 2024
@jancajthaml
Copy link

Any update or plan to tackle this issue?

@layershifter
Copy link
Contributor

layershifter commented Jan 19, 2024

I kinda agree with :hover behavior as it's also how styled-components work:


However, the issue with :global() is a real problem.

I will work on the plugin to stylis as namespace plugin from it does not do what is expected and I had to write a plugin for Griffel before (https://github.com/microsoft/griffel/blob/45e2a6b41596862bc2c54efbcbe746dc041e025e/packages/core/src/runtime/stylis/globalPlugin.ts#L1).

@layershifter
Copy link
Contributor

The fix was done in Anber/wyw-in-js#37 and released in @wyw-in-js/*@0.2.3.

@Dozalex
Copy link

Dozalex commented Jan 23, 2024

@layershifter Thanks for your fix! But I see another problem with :global

If i do

const globals = css`
  :global() {
    body {
      .someClassName {
        color: red;
      }
    }
  }
`

then I get the error Failed to match :global() selector in ".someClassName". Please report a bug if it happens.

If i do

const globals = css`
  :global() {
    body.someClassName {
      color: red;
    }
  }
`

then no error.

It's not a big problem, but before version 6 it worked this way and that way.

@nihgwu
Copy link

nihgwu commented Jan 29, 2024

Ran into the same issue as @Dozalex and it's a big issue for me as I'm using nested style with &, don't know how to workaround it

UPDATE: here is the workaround

@layershifter
Copy link
Contributor

@Dozalex seems that my change should be improved 🤔 I will try to fix it next week until somebody will do it earlier 🐱

@nihgwu
Copy link

nihgwu commented Jan 30, 2024

can we just have a globalCss instead 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report 🦗 Issue is probably a bug, but it needs to be checked bundler: webpack 📦 Issue is related to webpack bundler needs: complete repro 🖥️ Issue need to have complete repro provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants