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

Variant nodes are appended to the document rather than a root #7207

Closed
43081j opened this issue Jan 25, 2022 · 6 comments · Fixed by #7291
Closed

Variant nodes are appended to the document rather than a root #7207

43081j opened this issue Jan 25, 2022 · 6 comments · Fixed by #7291

Comments

@43081j
Copy link

43081j commented Jan 25, 2022

What version of Tailwind CSS are you using?

3.0.16

What build tool (or framework if it abstracts the build tool) are you using?

postcss-cli 9.1.0

What version of Node.js are you using?

17.x

Reproduction URL

See the reproduction in 43081j/postcss-lit#26


postcss recently introduced the concept of a "document". This allows custom syntaxes to be written for it which represent their arbitrary AST as the document, containing one or more Root nodes which are the actual CSS from said document.

For example, CSS in JS solutions would result in a Document representing the JS file, and one or more Root representing the contained stylesheets.

When combined with tailwind, this works like so:

  • postcss executes the custom syntax against the file, resolving to a postcss-compatible AST
  • postcss executes tailwind on this AST (Document { nodes: [Root, Root] } at this point)

Tailwind then executes code like this to replace the at-rules:

if (layerNodes.base) {
layerNodes.base.before(cloneNodes([...baseNodes, ...defaultNodes], layerNodes.base.source))
layerNodes.base.remove()
}

This works because layerNode.base.parent is one of the child Root nodes. So by calling before, we're prepending child nodes to the Root, not the Document (GOOD).

However, we can then reach this:

} else if (variantNodes.length > 0) {
root.append(cloneNodes(variantNodes, root.source))
}

Which, as you see, will append explicitly to root: the Document in this case.

This will result in an AST like so:

Document {
  nodes: [
    Root,
    Root,
    Rule, // BAD NODE!
  ]
}

When a custom syntax then attempts to stringify the AST back to the original non-css document, this misplaced Rule will be thrown on the end... causing a syntax error in the output.

Solution?

I don't know tailwind's source well enough to suggest a solution here. Possibly, if any layerNodes.* exists, use its parent as the root. If none exist, fall back to what it does now?

Or is there maybe an existing way to explicitly mark where those rules should exist? wherever layerNodes.variants comes from. Can we simply put some at-rule like @tailwindvariants?

Until this is fixed, custom postcss syntaxes can't really work with "variant rules".

@43081j
Copy link
Author

43081j commented Jan 25, 2022

/**
 * Use this directive to control where Tailwind injects the hover, focus,
 * responsive, dark mode, and other variants of each class.
 *
 * If omitted, Tailwind will append these classes to the very end of
 * your stylesheet by default.
 */
@tailwind variants;

@ma-g-ma maybe this is it! and i've just been blind. can you give it a go? add @tailwind variants to your source CSS

if it is, tailwind people: feel free to close this. we will just have to remember to always define the variants at-rule. i'll also update our syntax's docs

@ma-g-ma
Copy link

ma-g-ma commented Jan 25, 2022

@43081j Yep, it works! Both variants are correctly placed.

Though it seems that they appear with an invalid pseudo selector e.g.

.focus\:ring-blue-500:focus {
        --tw-ring-opacity: 1;
        --tw-ring-color: rgb(59 130 246 / var(--tw-ring-opacity))
}

IDE error messsage:

Unknown pseudo selector 'ring-blue-500'

Of course, this might be unrelated.

@43081j
Copy link
Author

43081j commented Jan 25, 2022

awesome.

for tailwind maintainers: maybe its still worth warning in these situations? if someone tries to use one of your classes without specifying the at-rule, especially if they have a Document node, its probably not their intention to dump it on the end. In those cases it would be useful to have a warning somehow. If they don't have a Document, no warning would be needed.

@ma-g-ma i'll track it in the other issue we have open and try look into it soon 👍

@vitorrd
Copy link
Contributor

vitorrd commented Jan 27, 2022

@43081j Yep, it works! Both variants are correctly placed.

Though it seems that they appear with an invalid pseudo selector e.g.

.focus\:ring-blue-500:focus {
        --tw-ring-opacity: 1;
        --tw-ring-color: rgb(59 130 246 / var(--tw-ring-opacity))
}

IDE error messsage:

Unknown pseudo selector 'ring-blue-500'

Of course, this might be unrelated.

Just commenting to say that the syntax you mentioned above is correct, as you can check through the W3C CSS Validator. Are you still having that issue?

@43081j
Copy link
Author

43081j commented Jan 27, 2022

That was a bug in the syntax FYI, backslashes not being escaped in the stringified output (it's in a string so one backslash would be nothing). It has been fixed.

I don't think there's necessarily a bug here in tailwind after all. But I do think the behaviour could be improved so the user gets warned when trying to use classes without the associated directive (only if they have a Document in their AST, implying it's a custom syntax).

@thecrypticace
Copy link
Contributor

The fix for this will go out in the next release. What we've opted to do for now is run Tailwind on each "root" node present in the postcss Document node. This should place variants at the right spot based on where we see any @tailwind directives. Once related please do let us know of any issues you encounter with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants