Skip to content
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

[Modal] - migrate to new Modal from deprecated #808

Closed
tlabaj opened this issue Dec 3, 2024 · 2 comments
Closed

[Modal] - migrate to new Modal from deprecated #808

tlabaj opened this issue Dec 3, 2024 · 2 comments
Assignees

Comments

@tlabaj
Copy link
Contributor

tlabaj commented Dec 3, 2024

Follow up to breaking change PR patternfly/patternfly-react#10799

Can we write a code mod to migrate from the old modal to the new composable version?

Required actions:

  1. Build codemod
  2. Build test
  3. Update readme with description & example
@thatblindgeye
Copy link
Collaborator

thatblindgeye commented Dec 19, 2024

Since this was made as a spike, I'll report my thoughts/findings here.

Basically, I think this is possible, there's just several moving parts to consider.

We should take into account 1) consumers who have already ran codemods for a v6 migration and have had deprecated Modals updated to the deprecated directory import path, and 2) consumers who have not yet ran codemods

For Group 1, that would entail updating the import path back to the non-deprecated import directory -- this would most likely in turn require updating the moveSpecifiers helper to a tsx file type, but also updating it in such a way that it can be used inline instead of just something you pass to a codemods create method. The alternative to this for now would be just updating the declaration source to remove "deprecated" from the path -- basically resulting in 2 import declarations from patternfly/react-core which the consumer has to deal with (but could be refactored whenever a separate effort for typing moveSpecifiers is done).

For Group 2, that would entail checking to see if there's a data-codemods comment (such as if a consumer was using the previous Next implementation, then ran codemods to update import paths to just patternfly/react-core which leaves such a comment; likely? possibly not, but we still need this logic). If a comment exists, do nothing because they should be all set.

For both groups, we would then just a) add all of the subcomponents to the react import, b) grab several props that could have been passed to the deprecated Modal but are now meant to be passed to another sub-component (e.g. titleLabel now passed to ModalHeader), as well as construct the children of Modal based on such props (e.g. if hasNoBodyWrapper was previously passed on Modal, we shouldn't use the ModalBody component)... and c) add a comment to the Modal specifier so that this codemod will get skipped on subsequent runs.

Any imports we added that weren't used would be removed from the no-unused-import rule (not ideal, but I feel is a little better than trying to traverse a file in the ImportDeclaration block and determine what needs to be imported from there; no strong opinion on this, though).

One consideration is the fact that the deprecated Modal I believe did export a bunch of sub-components for consumers to build a composable implementation if they wanted. Is that something we want to take into account, and thus have another set of logic to deal with (e.g. having to check if DeprecatedSubComponent was used and try to figure out what current subcomponent needs to be used instead). If not, is it better to explicitly state that a codemod for this is meant for non-composable deprecated Modals -- which might be better if we make such a codemod require the --only flag. This also could be something to worry about only if it's brought up, as I'm not sure how many consumers really used a composable approach of the deprecated Modal.

@thatblindgeye
Copy link
Collaborator

Notes from @wise-king-sullyman:

  • Narrow scope for initial implementation -- focus on updating a deprecated import back to non-deprecated
    • Moving to deprecated is good enough for v6 migration -- this gives us time to work on this potential codemod, especially because part of this effort requires typing/refactoring the moveSpecifiers helper
  • This could technically be the first codemod for v7
  • We would tell consumers not to worry about the deprecated import of Modal for now; something will come down the pipeline to help automate updating to the current, composable implementation
  • We should check analytics to see how many consumers are using composability of deprecated Modal

Will open a couple separate issues from all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants