-
Notifications
You must be signed in to change notification settings - Fork 431
feat(upgrade): add satellite auto-sync codemod for core 3 migration #7653
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
Conversation
Why: Core 3 changed satellite behavior — apps no longer auto-sync on first visit unless `satelliteAutoSync: true` is set. This codemod preserves Core 2 behavior by adding the prop wherever `isSatellite` is configured.
🦋 Changeset detectedLatest commit: 650976b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughAdds a new codemod named 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/upgrade/src/codemods/transform-satellite-auto-sync.cjs`:
- Around line 6-47: The object transform in root.find(j.ObjectExpression)
currently finds isSatelliteIndex and unconditionally inserts satelliteAutoSync
at isSatelliteIndex + 1, which can be shadowed by a following spread; update the
logic to detect any SpreadElement/SpreadProperty (AST node representing ...obj)
after the isSatellite property: if a spread exists after isSatellite, either
skip the transform or insert the new j.objectProperty('satelliteAutoSync',
j.booleanLiteral(true)) before the first spread instead of at isSatelliteIndex +
1; adjust the check that uses isObjectPropertyNamed(...) and the insertion that
calls properties.splice(...) to use the computed safe insert index (or bail out)
so explicit property values are not overridden by spreads.
Ephem
left a comment
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.
I posted a question about false positives, but I think we are fine with that tradeoff and implementation looks good!
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.
Is adding satelliteAutoSync to every component and object that has a isSatellite overly broad, given there might be unrelated components/objects that also has it?
I get this is tricky to narrow down given props and objects can be passed around, is the idea that we'd rather have false positives than miss some?
Seems like an edge case, should still be safe and most people review the output after running a codemod anyway so I'm very fine with this, just wanted to double check this is the intent?
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.
Thanks for the review Fredrik :) I thought about this as well and I decided that I could live with false positives given how impactful the change in behavior could be for apps migrating from Core 2 to Core 3. Happy to change this of course
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.
Thinking more about this and your comment below, I decided to align the behavior and always add the prop at the end - I do think its important for as to override always, and let the end user decide in the very rare event that they have already added satelliteAutoSync manually (very unlikely as this prop will be introduced with core3)
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.
Very true, sounds good to me! 🙏
Why: Ephem's review noted JSX and object transforms behaved inconsistently with spreads. JSX appended to end (codemod wins), objects inserted after isSatellite (spread could win). For an important migration, we want the codemod value to always apply so users explicitly review and remove if needed, rather than silently skipping. What changed: Object transform now uses push() instead of splice() to append satelliteAutoSync at the end, matching JSX behavior. Both transforms now consistently ensure the codemod value takes precedence over spreads.
Why
Core 3 introduced
satelliteAutoSync(defaults tofalse), changing satellite behavior. In Core 2, satellite apps always auto-synced on first visit. In Core 3, they don't unlesssatelliteAutoSync: trueis explicitly set. Users upgrading need this prop added automatically to preserve existing behavior.What changed
transform-satellite-auto-sync.cjscodemod that findsisSatelliteusage (both JSX props and object properties) and addssatelliteAutoSync: trueas a siblingsatelliteAutoSyncis already presentPackages affected
@clerk/upgrade: New codemod + testsSummary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.