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

[docs] Update the "remove props" example #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexfauquette
Copy link

The remove prop example suffers one issue: nested components

If you have two nested components with the same prop it will be removed to both of them.

For example

<A prop="test">
  <B prop="test2" />
</A>

Using the example to remove prop from <A /> Will also remove it from <B /> because find also cover the nested elements

The remove prop example suffers one issue: nested components

If you have two nested components with the same prop it will be removed to both of them.

For example

```jsx
<A prop="test">
  <B prop="test2" />
</A>
```

Using the example to remove `prop` from `<A />` Will also remove it from `<B />` because find also cover the nested elements
Copy link
Contributor

@danieldelcore danieldelcore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alexfauquette thanks for raising a PR – Much appreciated!
Left a comment for you, let me know what you think!

const attributeParent = attribute.parentPath.parentPath;
if (
attributeParent.value.type === 'JSXOpeningElement' &&
componentNames.includes(attributeParent.value.name.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually an expected behaviour for this example ie: // Find all button jsx elements & // Find all attributes (props) on the button. (although, I should probably have done a better job at explaining the specifics of the approach).

In this case, non-button elements will not be targeted by the codemod.

import React from 'react';

const Button = props => {
-  return <button className="button" onClick={() => console.log('Hello, World!')}>
+  return <button className="button">
    <div onClick={() => {}}> // This line is unchanged
      Submit
    </div>
  </button>;
};

The component filtering logic happens on line 71. It's fairly barebones for example purposes, but if you wanted to be more specific here you could update that to target only the components you want to target.

-    .filter(path => path.value.openingElement.name.name === 'button') // Find all button jsx elements
+    .filter(path => path.value.openingElement.name.name === 'MyCustomButton') // Find all custom buttons jsx 

Potentially, we could create another example here instead to explicitly show how this could be applied to JSX elements and how filtering results in only the targetted elements being affected. Would you like to give that a go?

I might see if I can add some descriptions to the examples!

By the way, is componentNames just for example purposes here?

Copy link
Author

@alexfauquette alexfauquette Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the current example:

export default function transformer(file, { jscodeshift: j }, options) {
  const source = j(file.source);

  source
    .find(j.JSXElement)
    .filter((path) => path.value.openingElement.name.name === 'button') // Find all button jsx elements
    .find(j.JSXAttribute) // Find all attributes (props) on the button
    .filter((path) => path.node.name.name === 'onClick') // Filter to only props called onClick
    .remove(); // Remove everything that matched

  return source.toSource();
}

On the nested button/div you provided:

import React from 'react';

const Button = (props) => {
  return (
    <button className="button" onClick={() => console.log('Hello, World!')}>
      <div onClick={() => {}}> // This line is unchanged Submit</div>
    </button>
  );
};

The result of the codemod removes the onClick on both: the button and the div. Maybe I'm missing something but I don't get what 🤔

I opened a draft PR to let you see the code I'm running

@danieldelcore
Copy link
Contributor

BTW, if you need any integration help or codemods for MUI, feel free to reach out to me on Twitter or Discord I'd love to collaborate with you folks!

@alexfauquette
Copy link
Author

BTW, if you need any integration help or codemods for MUI, feel free to reach out to me on Twitter or Discord I'd love to collaborate with you folks!

Thanks, I just finished writing the codemods for v5 to v6 migration for pickers. I transmitted your interest to the rest of the team :)

@danieldelcore danieldelcore force-pushed the main branch 2 times, most recently from facba25 to 5248c0e Compare July 15, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants