Ensure it's safe to perform suffix-less migrations #14979
Merged
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
ecc48ad to
ac11740
Compare
1350885 to
f005932
Compare
philipp-spiess
requested changes
Nov 13, 2024
Member
philipp-spiess
left a comment
There was a problem hiding this comment.
LGTM! One comment about the moved printCandidate which I think should be moved back into the condition (otherwise we force every candidate through the candidate deseiralization/serialization logic.
Comment on lines
41
to
45
|
|
||
| // The printCandidate function will already put the exclamation mark in the | ||
| // right place, so we just need to mark this candidate as requiring a | ||
| // migration. | ||
| return printCandidate(designSystem, candidate) |
Member
There was a problem hiding this comment.
Let's move this back inside the if condition, otherwise all candidates that are not ending with an exclamation mark are force-reprinted which does seem unexpected (e.g. if we have a serialize/deserialize issue it now affects all candidates)
Member
Author
There was a problem hiding this comment.
Aha, I messed it up due to the refactor and deleted the wrong }. Fixed now!
Had incorrect location of closing `}` due to refactor. This fixes it.
philipp-spiess
approved these changes
Nov 13, 2024
Co-authored-by: Philipp Spiess <hello@philippspiess.com>
RobinMalfait
added a commit
that referenced
this pull request
Nov 13, 2024
This PR makes sure that migrations from suffix-less candidates (e.g.:
`rounded`, `blur`, `shadow`) are safe to be migrated.
In some code snippets that's not always the case.
Given the following code snippet:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded shadow blur" filter={blur ? 'url(…)' : undefined} />
}
```
Without this change, it would result in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur-sm?: boolean,
rounded-sm?: boolean,
shadow-sm?: boolean,
]
function Star({ point: [cx, cy, dim, blur-sm, rounded-sm, shadow-sm] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur-sm ? 'url(…)' : undefined} />
}
```
But with this change, it results in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur ? 'url(…)' : undefined} />
}
```
Notice how the classes inside the `class` attribute _are_ converted, but
the ones in the types or as part of the JavaScript code (e.g.:
`filter={blur ? 'url(…)' : undefined}`) are not.
---------
Co-authored-by: Philipp Spiess <hello@philippspiess.com>
RobinMalfait
added a commit
that referenced
this pull request
Nov 13, 2024
This PR makes sure that migrations from suffix-less candidates (e.g.:
`rounded`, `blur`, `shadow`) are safe to be migrated.
In some code snippets that's not always the case.
Given the following code snippet:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded shadow blur" filter={blur ? 'url(…)' : undefined} />
}
```
Without this change, it would result in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur-sm?: boolean,
rounded-sm?: boolean,
shadow-sm?: boolean,
]
function Star({ point: [cx, cy, dim, blur-sm, rounded-sm, shadow-sm] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur-sm ? 'url(…)' : undefined} />
}
```
But with this change, it results in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur ? 'url(…)' : undefined} />
}
```
Notice how the classes inside the `class` attribute _are_ converted, but
the ones in the types or as part of the JavaScript code (e.g.:
`filter={blur ? 'url(…)' : undefined}`) are not.
---------
Co-authored-by: Philipp Spiess <hello@philippspiess.com>
RobinMalfait
added a commit
that referenced
this pull request
Nov 14, 2024
This PR makes sure that migrations from suffix-less candidates (e.g.:
`rounded`, `blur`, `shadow`) are safe to be migrated.
In some code snippets that's not always the case.
Given the following code snippet:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded shadow blur" filter={blur ? 'url(…)' : undefined} />
}
```
Without this change, it would result in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur-sm?: boolean,
rounded-sm?: boolean,
shadow-sm?: boolean,
]
function Star({ point: [cx, cy, dim, blur-sm, rounded-sm, shadow-sm] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur-sm ? 'url(…)' : undefined} />
}
```
But with this change, it results in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur ? 'url(…)' : undefined} />
}
```
Notice how the classes inside the `class` attribute _are_ converted, but
the ones in the types or as part of the JavaScript code (e.g.:
`filter={blur ? 'url(…)' : undefined}`) are not.
---------
Co-authored-by: Philipp Spiess <hello@philippspiess.com>
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 makes sure that migrations from suffix-less candidates (e.g.:
rounded,blur,shadow) are safe to be migrated.In some code snippets that's not always the case.
Given the following code snippet:
Without this change, it would result in:
But with this change, it results in:
Notice how the classes inside the
classattribute are converted, but the ones in the types or as part of the JavaScript code (e.g.:filter={blur ? 'url(…)' : undefined}) are not.