Fix template migration issues #14600
Merged
Conversation
This file contains 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
philipp-spiess
commented
Oct 4, 2024
| ): Promise<{ rawCandidate: string; start: number; end: number }[]> { | ||
| let scanner = new Scanner({}) | ||
| let result = scanner.getCandidatesWithPositions({ content, extension: 'html' }) | ||
| let result = scanner.getCandidatesWithPositions({ content, extension }) |
There was a problem hiding this comment.
Note to myself: Let's try to see if we can find out why this emits duplicates
There was a problem hiding this comment.
Wasn't really able to repro this in unit tests 🤔
There was a problem hiding this comment.
We can chat about this some time today but I'd guess one of the routines we use to slice up a candidate into multiple candidates produces two identical entries.
There was a problem hiding this comment.
@thecrypticace Sounds good yeah, I was trying to reproduce it inside Rust unit tests to no avail. I wonder if this has to do with the fact that we have two files in the repro—I have a feeling that there's some shared state but I didn't see anything that stood out.
philipp-spiess
force-pushed
the
fix/fix-template-migration-issues
branch
from
a79a96e to
cd5f389
Compare
adamwathan
reviewed
Oct 4, 2024
philipp-spiess
force-pushed
the
fix/fix-template-migration-issues
branch
from
cd5f389 to
9ba6a8d
Compare
philipp-spiess
force-pushed
the
fix/fix-template-migration-issues
branch
from
1296b04 to
1936261
Compare
philipp-spiess
force-pushed
the
fix/fix-template-migration-issues
branch
from
788c380 to
43fe42b
Compare
adamwathan
approved these changes
Oct 8, 2024
philipp-spiess
commented
Oct 8, 2024
RobinMalfait
pushed a commit
that referenced
this pull request
Oct 9, 2024
This PR fixes two issues we found when testing the candidate codemodes: 1. Sometimes, core would emit the same candidate twice. This would result into rewriting a range multiple times, without realizing that this change might already be applied, causing it to swallow or duplicate some bytes. 2. The codemods were mutating the `Candidate` object, however since the `Candidate` parsing is _cached_ in core, it would sometimes return the same instance. This is an issue especially since we monkey patch the prefix to `null` when migrating prefixed candidates. This means that a candidate would be cached that would be _invalid relative to the real design system_. We fixed this by making sure the mutations would only be applied to clones of the `Candidate` and I changed the `DesignSystem` API to return `ReadOnly<T>` versions of these candidates. A better solution would maybe be to disable the cache at all but this requires broader changes in Core.
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 two issues we found when testing the candidate codemodes:
Candidateobject, however since theCandidateparsing is cached in core, it would sometimes return the same instance. This is an issue especially since we monkey patch the prefix tonullwhen migrating prefixed candidates. This means that a candidate would be cached that would be invalid relative to the real design system. We fixed this by making sure the mutations would only be applied to clones of theCandidateand I changed theDesignSystemAPI to returnReadOnly<T>versions of these candidates. A better solution would maybe be to disable the cache at all but this requires broader changes in Core.