-
Notifications
You must be signed in to change notification settings - Fork 453
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a0ecf5d
feat(upgrade): add satellite auto-sync codemod for core 3 migration
nikosdouvlis b4d0d96
chore(upgrade): add changeset
nikosdouvlis 863c56c
refactor(upgrade): append satelliteAutoSync at end of objects
nikosdouvlis 650976b
Merge branch 'main' into nikos/satellite-auto-sync-codemod
nikosdouvlis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
173 changes: 173 additions & 0 deletions
173
...ges/upgrade/src/codemods/__tests__/__fixtures__/transform-satellite-auto-sync.fixtures.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| export const fixtures = [ | ||
| { | ||
| name: 'JSX: isSatellite={true}', | ||
| source: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider isSatellite={true} domain="example.com"> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| output: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider isSatellite={true} domain="example.com" satelliteAutoSync={true}> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| }, | ||
| { | ||
| name: 'JSX: isSatellite (boolean shorthand)', | ||
| source: ` | ||
| import { ClerkProvider } from '@clerk/react'; | ||
|
|
||
| export const App = () => ( | ||
| <ClerkProvider isSatellite domain="satellite.example.com"> | ||
| <Main /> | ||
| </ClerkProvider> | ||
| ); | ||
| `, | ||
| output: ` | ||
| import { ClerkProvider } from '@clerk/react'; | ||
|
|
||
| export const App = () => ( | ||
| <ClerkProvider isSatellite domain="satellite.example.com" satelliteAutoSync={true}> | ||
| <Main /> | ||
| </ClerkProvider> | ||
| ); | ||
| `, | ||
| }, | ||
| { | ||
| name: 'JSX: isSatellite with function value', | ||
| source: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider isSatellite={(url) => url.host === 'satellite.example.com'} domain="example.com"> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| output: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider | ||
| isSatellite={(url) => url.host === 'satellite.example.com'} | ||
| domain="example.com" | ||
| satelliteAutoSync={true}> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| }, | ||
| { | ||
| name: 'JSX: already has satelliteAutoSync', | ||
| source: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider isSatellite={true} satelliteAutoSync={false} domain="example.com"> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| noChange: true, | ||
| }, | ||
| { | ||
| name: 'Object: isSatellite in clerkMiddleware options', | ||
| source: ` | ||
| import { clerkMiddleware } from '@clerk/nextjs/server'; | ||
|
|
||
| export default clerkMiddleware({ | ||
| isSatellite: true, | ||
| domain: 'example.com', | ||
| }); | ||
| `, | ||
| output: ` | ||
| import { clerkMiddleware } from '@clerk/nextjs/server'; | ||
|
|
||
| export default clerkMiddleware({ | ||
| isSatellite: true, | ||
| satelliteAutoSync: true, | ||
| domain: 'example.com' | ||
| }); | ||
| `, | ||
| }, | ||
| { | ||
| name: 'Object: isSatellite in variable declaration', | ||
| source: ` | ||
| const options = { | ||
| isSatellite: true, | ||
| domain: 'satellite.example.com', | ||
| }; | ||
| `, | ||
| output: ` | ||
| const options = { | ||
| isSatellite: true, | ||
| satelliteAutoSync: true, | ||
| domain: 'satellite.example.com' | ||
| }; | ||
| `, | ||
| }, | ||
| { | ||
| name: 'Object: isSatellite with function value', | ||
| source: ` | ||
| import { clerkMiddleware } from '@clerk/nextjs/server'; | ||
|
|
||
| export default clerkMiddleware({ | ||
| isSatellite: (url) => url.host === 'satellite.example.com', | ||
| domain: 'example.com', | ||
| }); | ||
| `, | ||
| output: ` | ||
| import { clerkMiddleware } from '@clerk/nextjs/server'; | ||
|
|
||
| export default clerkMiddleware({ | ||
| isSatellite: (url) => url.host === 'satellite.example.com', | ||
| satelliteAutoSync: true, | ||
| domain: 'example.com' | ||
| }); | ||
| `, | ||
| }, | ||
| { | ||
| name: 'Object: already has satelliteAutoSync', | ||
| source: ` | ||
| const options = { | ||
| isSatellite: true, | ||
| satelliteAutoSync: false, | ||
| domain: 'example.com', | ||
| }; | ||
| `, | ||
| noChange: true, | ||
| }, | ||
| { | ||
| name: 'No isSatellite present (no changes)', | ||
| source: ` | ||
| import { ClerkProvider } from '@clerk/nextjs'; | ||
|
|
||
| export function App({ children }) { | ||
| return ( | ||
| <ClerkProvider domain="example.com"> | ||
| {children} | ||
| </ClerkProvider> | ||
| ); | ||
| } | ||
| `, | ||
| noChange: true, | ||
| }, | ||
| ]; |
17 changes: 17 additions & 0 deletions
17
packages/upgrade/src/codemods/__tests__/transform-satellite-auto-sync.test.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { applyTransform } from 'jscodeshift/dist/testUtils'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import transformer from '../transform-satellite-auto-sync.cjs'; | ||
| import { fixtures } from './__fixtures__/transform-satellite-auto-sync.fixtures'; | ||
|
|
||
| describe('transform-satellite-auto-sync', () => { | ||
| it.each(fixtures)('$name', ({ source, output, noChange }) => { | ||
| const result = applyTransform(transformer, {}, { source }); | ||
|
|
||
| if (noChange) { | ||
| expect(result).toEqual(''); | ||
| } else { | ||
| expect(result).toEqual(output.trim()); | ||
| } | ||
| }); | ||
| }); |
74 changes: 74 additions & 0 deletions
74
packages/upgrade/src/codemods/transform-satellite-auto-sync.cjs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| module.exports = function transformSatelliteAutoSync({ source }, { jscodeshift: j }) { | ||
| const root = j(source); | ||
| let dirty = false; | ||
|
|
||
| // Handle JSX attributes: <Component isSatellite /> → <Component isSatellite satelliteAutoSync={true} /> | ||
| root.find(j.JSXOpeningElement).forEach(path => { | ||
| const { attributes } = path.node; | ||
| if (!attributes) { | ||
| return; | ||
| } | ||
|
|
||
| const hasIsSatellite = attributes.some(attr => isJsxAttrNamed(attr, 'isSatellite')); | ||
| if (!hasIsSatellite) { | ||
| return; | ||
| } | ||
|
|
||
| const hasAutoSync = attributes.some(attr => isJsxAttrNamed(attr, 'satelliteAutoSync')); | ||
| if (hasAutoSync) { | ||
| return; | ||
| } | ||
|
|
||
| const autoSyncAttr = j.jsxAttribute( | ||
| j.jsxIdentifier('satelliteAutoSync'), | ||
| j.jsxExpressionContainer(j.booleanLiteral(true)), | ||
| ); | ||
| attributes.push(autoSyncAttr); | ||
| dirty = true; | ||
| }); | ||
|
|
||
| // Handle object properties: { isSatellite: true } → { isSatellite: true, satelliteAutoSync: true } | ||
| root.find(j.ObjectExpression).forEach(path => { | ||
| const { properties } = path.node; | ||
|
|
||
| const hasIsSatellite = properties.some(prop => isObjectPropertyNamed(prop, 'isSatellite')); | ||
| if (!hasIsSatellite) { | ||
| return; | ||
| } | ||
|
|
||
| const hasAutoSync = properties.some(prop => isObjectPropertyNamed(prop, 'satelliteAutoSync')); | ||
| if (hasAutoSync) { | ||
| return; | ||
| } | ||
|
|
||
| const isSatelliteIndex = properties.findIndex(prop => isObjectPropertyNamed(prop, 'isSatellite')); | ||
| const autoSyncProp = j.objectProperty(j.identifier('satelliteAutoSync'), j.booleanLiteral(true)); | ||
| properties.splice(isSatelliteIndex + 1, 0, autoSyncProp); | ||
| dirty = true; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| return dirty ? root.toSource() : undefined; | ||
| }; | ||
|
|
||
| module.exports.parser = 'tsx'; | ||
|
|
||
| function isJsxAttrNamed(attribute, name) { | ||
| return attribute && attribute.type === 'JSXAttribute' && attribute.name && attribute.name.name === name; | ||
| } | ||
|
|
||
| function isObjectPropertyNamed(prop, name) { | ||
| if (!prop || (prop.type !== 'ObjectProperty' && prop.type !== 'Property')) { | ||
| return false; | ||
| } | ||
| const { key } = prop; | ||
| if (!key) { | ||
| return false; | ||
| } | ||
| if (key.type === 'Identifier') { | ||
| return key.name === name; | ||
| } | ||
| if (key.type === 'StringLiteral' || key.type === 'Literal') { | ||
| return key.value === name; | ||
| } | ||
| return false; | ||
| } | ||
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
Oops, something went wrong.
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.
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
satelliteAutoSyncto every component and object that has aisSatelliteoverly 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! 🙏