ParabolInc / parabol

Free online agile retrospective meeting tool
https://www.parabol.co/
Other
1.91k stars 327 forks source link

Replace `createFragmentContainer` with `useFragment` #6283

Closed Dschoordsch closed 1 year ago

Dschoordsch commented 2 years ago

createFragmentContainer is deprecated and should be replaced with useFragment. See Partial data for details.

Use the codeshift automation described in the migration doc to update most of the instances to the hook.

Acceptance criteria

jordanh commented 2 years ago

Could we divide this into smaller issues? Perhaps re-write the AC to 1) discover all the places to refactor 2) open issues to break this refactoring into chunks?

jmtaber129 commented 2 years ago

My 2¢ as I'm currently doing a similar (but at a much smaller scale and with less complexity) HOC->hook migration for withAtmosphere - there's currently 276 occurrences of createFragmentContainer(, so ~226 components need to be migrated. This is a lot to change manually (it took me the better part of an hour to manually migrate 20(!) React class components to function components - a supposedly trivial migration in most cases), so I would strongly recommend automating this via something like jscodeshift. Even if we decide to approach this primarily through "in the neighborhood" refactoring, we should try to automate most of this to reduce the friction associated with changing one of these components - I don't want an engineer to have to take the time to read a doc on this to figure out how to manually perform the migration. I'd rather engineers just be told "run this script if you're changing a component with createFragmentContainer, then go do whatever changes you want on that component".

jmtaber129 commented 2 years ago

FWIW I have some interest in picking this up (it's been a while since I've played with Babel+ASTs [shameless plug] and I've been meaning to check out jscodeshift), though I may not have capacity for a few sprints.

jordanh commented 2 years ago

@jmtaber129 I think building some AST jscodeshift chops could be real, real nice. Discuss on your squad or pick up during a slack week!

jmtaber129 commented 2 years ago

(reopening since the closing PR only added automation to assist this effort; the AC still needs to be executed)

github-actions[bot] commented 1 year ago

Stale issue

jordanh commented 1 year ago

Was about to icebox, but I think we've done most of the work here to automate this migration. Will keep open a bit longer.

@jmtaber129 please let me know if you'd like to add anything to this Issue so that we prioritize it or decide to close it

jmtaber129 commented 1 year ago

Yea, all the automation is implemented, we just need to actually apply it.

I'll probably run a few batch migrations in the next few weeks, so let's keep it out of the icebox