Skip to content

Ensure multiple --value(…) or --modifier(…) calls don't delete subsequent declarations #17273

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.

Sign up for GitHub

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

Merged
RobinMalfait merged 5 commits into from
Mar 19, 2025
+37 −1
Merged

Ensure multiple --value(…) or --modifier(…) calls don't delete subsequent declarations #17273

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
255b877
add failing test
RobinMalfait Mar 18, 2025
5d501bc
safety: throw error when we call `replaceNode(…)` multiple times
RobinMalfait Mar 18, 2025
7c104b9
stop traversing once we replace the node
RobinMalfait Mar 18, 2025
db4b9e7
update changelog
RobinMalfait Mar 18, 2025
9501584
silently bail
RobinMalfait Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
safety: throw error when we call replaceNode(…) multiple times 
Other solution is to silently ignore, but that feels even more buggy.
  • Loading branch information
@RobinMalfait
RobinMalfait committed Mar 19, 2025
commit 5d501bc52b9715336ba138d0bb65a0724b1e214b
3 changes: 3 additions & 0 deletions packages/tailwindcss/src/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ export function walk(
context,
path,
replaceWith(newNode) {
if (replacedNode) {
throw new Error('Cannot replace a node more than once')
}
Copy link
Member Author

@RobinMalfait RobinMalfait Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also silently ignore this but then we are just hiding a bug.

Maybe we could write a better error message here that this is a Tailwind internal bug? /cc @adamwathan

Copy link
Member

@adamwathan adamwathan Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just not throw here? Is there something the user can do that triggers this or does it only signal a bug in Tailwind?

Copy link
Member Author

@RobinMalfait RobinMalfait Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing the user can do and signals a bug in Tailwind for sure because we are the only ones using this API.

Only issue about silently ignoring is that we wouldn't immediately know about an issue. And worse, maybe some APIs start relying on this odd behavior.

If we had thrown here than the bug you ran into would be obvious but would also break everything 🫣

Copy link
Contributor

@thecrypticace thecrypticace Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropping in a suggestion here:

Cannot replace a node more than once. This is a bug in Tailwind CSS. Please open an issue with a reproduction at https://github.com/tailwindlabs/tailwindcss/issues

Copy link
Member

@adamwathan adamwathan Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there are probably a million places in the code where we could add conditionals like this if we really wanted right? Feels weird to me that this would be the first time we ever introduced a throw to tell us we made a mistake. I don’t think we’ve ever done this any other time we fixed a bug.

Copy link
Member Author

@RobinMalfait RobinMalfait Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense. I silently bailed so at least we are not mutating other AST nodes.

replacedNode = true

if (Array.isArray(newNode)) {
Expand Down
3 changes: 3 additions & 0 deletions packages/tailwindcss/src/compat/selector-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ export function walk(
visit(node, {
parent,
replaceWith(newNode) {
if (replacedNode) {
throw new Error('Cannot replace a node more than once')
}
replacedNode = true

if (Array.isArray(newNode)) {
Expand Down
3 changes: 3 additions & 0 deletions packages/tailwindcss/src/value-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ export function walk(
visit(node, {
parent,
replaceWith(newNode) {
if (replacedNode) {
throw new Error('Cannot replace a node more than once')
}
replacedNode = true

if (Array.isArray(newNode)) {
Expand Down