-
Notifications
You must be signed in to change notification settings - Fork 179
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
TypeScript: Migrate migration
package
#12320
Conversation
# Conflicts: # packages/patterns/src/generatePatternStyles.ts
@@ -55,7 +55,7 @@ describe('setOpacity', () => { | |||
elements: [ | |||
{ | |||
_test: 'element1', | |||
opacity: 100, | |||
opacity: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opacity was set but 0
so it should stay as it was.
}, | ||
{ | ||
_test: 'element2', | ||
type: 'text', | ||
backgroundColor: 'transparent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background color can't be transparent
at this point, it was migrated to Pattern
in V6
src: 'https://example.com/audio.mp3', | ||
id: 123, | ||
mimeType: 'audio/mpeg', | ||
resource: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Story background audio was migrated to have resource
in V39.
|
||
function updateElement(element: UnionElementV6): UnionElementV7 { | ||
// If it's a text element or already has flip set, return as is. | ||
if ('content' in element || 'flip' in element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a better way for TS to understand it's a text element, so using 'content' in element
since that's unique to text element at this point.
Plugin builds for 7a94718 are ready 🛎️!
|
Size Change: +276 B (0%) Total Size: 2.71 MB
ℹ️ View Unchanged
|
packages/migration/src/migrate.ts
Outdated
import removeTagNames from './migrations/v0043_removeTagNames'; | ||
|
||
const MIGRATIONS = { | ||
const MIGRATIONS: Record<number, any[]> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still don't have a solution for a generic MigrationFn
type (instead of any[]
). Any (non-any
😛 ) ideas are appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a tough one. Unfortunately the article Morten shared before doesn't really cover that for a case like ours.
I've briefly looked into using union types here or something else clever, but couldn't find a nice way for it.
Soo... maybe we just use any
for now until we find something better? 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this has to do. I have a weird vision about how we might make a properly typed migration system, but I'll have to play around with it. Great effort so far!
Merging with failing Karma tests since those are unrelated. |
Context
Summary
Relevant Technical Choices
To-do
migrate.ts
file'smigrationFn
needs figuring outPageV[X]
andStoryV[X]
types.User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZ
label to the PRFixes #12239