-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ag 13133 codemod #104
Ag 13133 codemod #104
Conversation
138223a
to
2ba1db7
Compare
2ba1db7
to
5f425e1
Compare
columnDefs={[]} | ||
rowData={[]} | ||
defaultColDef={{ | ||
unSortIcon: true |
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.
Concerned that this is going to introduce extra renders due to defaultColDef not being memoized. If the defaultColDef is already on the grid like this then thats fine to mutate, but otherwise I feel like we need to follow our own advice from https://grid-staging.ag-grid.com/react-data-grid/react-hooks/
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.
Likewise - but again the complexity of the code mod scales too much to be able to handle this, the alternative option is no code-mod for this I think, unless you have some idea?
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 think we should not attempt this code mod if we are going to leave the user with sub optimal code.
|
||
function MyComponent10 (props) { | ||
const [groupRemoveLowestSingleChildren, setGroupRemoveLowestSingleChildren] = useState(true); | ||
return ( |
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.
Are we doing anything for Angular and Vue?
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.
No - but historically we haven't anyway, it seems Tims stuff didn't work for angular (correct me if I'm wrong?)
It also doesn't look to have any mechanism for handling this kind of change, correct me if I'm wrong? Without spending the time completely rewriting the code mods I'm not really sure what else to do
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.
Might just be a case of checking if the JS version tweaks Angular code that is defined in the component, in a way that then does not match the template. Same potentially for Vue, the code might be updated by the JS one too.
path.replace(); | ||
return; | ||
} | ||
if (thisValObj?.type === 'BooleanLiteral' && typeof thisValObj.value === 'boolean') { |
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.
minor, we could use jscodeshift's j.BooleanLiteral.check(encapsulatedValue)
here (and similar calls elsewhere where we're checking types)
No description provided.