Fix false-positive migrations in addEventListener and JavaScript variable names
#18718
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use test table such that if a single test fails, all tests after still execute.
To be a bit more correct and consistent with the `addEventListener` approach.
Our existing heuristic looked for a quote _somewhere_ before the
candidate and a quote (doesn't need to be the same type of quote)
_somewhere_ after the candidate.
While this is an okay assumption, it also has flaws, because this means
that the `outline` in the following example is considered inside of a
string:
```js
function foo({ a = "", outline = true, b = "" }) {
//
}
```
With this change, we will do a little bit of parsing and figure out if
we are in a string (by looking at balanced quotes).
| const BACKSLASH = 0x5c | ||
| const DOUBLE_QUOTE = 0x22 | ||
| const SINGLE_QUOTE = 0x27 | ||
| const BACKTICK = 0x60 |
There was a problem hiding this comment.
Should we consider that in JS backtick strings can span lines — or just not worry about that unless it comes up?
There was a problem hiding this comment.
We haven't in the past and nobody had issues with this yet (afaik at least) so I don't think we need to add the additional complexity of this (yet) 🤔
thecrypticace
approved these changes
Aug 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes 2 false-positives when running the upgrade tool on a Tailwind CSS v3 project converting it to a Tailwind CSS v4 project.
The issue occurs around migrations with short simple names that have a meaning outside if Tailwind CSS, e.g.
blurandoutline.This PR fixes 2 such cases:
The
addEventListenercase:We do this by special casing the
addEventListener(case and making sure the first argument toaddEventListeneris never migrated.A JavaScript variable with default value:
The bug is relatively subtle here, but it has actually nothing to do with
outlineitself, but rather the fact that some quote character came before and after it on the same line...One of our heuristics for determining if a migration on these small words is safe, is to ensure that the candidate is inside of a string. Since we didn't do any kind of quote balancing, we would consider the
outlineto be inside of a string, even though it is not.So to actually solve this, we do some form of quote balancing to ensure that it's not inside of a string in this case.
Additionally, this PR also introduces a small refactor to the
is-safe-migration.test.tsfile where we now use atest.eachto ensure that failing tests in the middle don't prevent the rest of the tests from running.Test plan
Fixes: #18675