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

feat: added codemod for renaming unsafe lifecycles in react #25

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

shubhraagarwal
Copy link

📚 Description

While following the command to rename unsafe lifecycles in react given here, I was presented with a command not found.

After discussing with the mods of react-codemod, I copied the existing codemod from that repository, adopted that code according to the codemod standards.

🔗 Linked Issue

https://github.com/reactjs/react-codemod/issues/329

🧪 Test Plan

📄 Documentation to Update

@@ -0,0 +1 @@
const toReplace = "hello";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the fixtures need to be updated, right?

Copy link
Author

Choose a reason for hiding this comment

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

updated, please take a look and let me know if it works or if any changes are needed

Copy link
Contributor

Choose a reason for hiding this comment

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

@shubhraagarwal I ran the tests but they seem to be failing. I can see discrepancies between what I would expect the output to probably be and the given output testfixture. Were you able to get the tests to pass before? By the way, I think it's much easier if you feed the codemod into Codemod Studio, place (and test) your test fixtures there (bottom panels), and then export the codemod package while being sure that the tests were passing.

@mohab-sameh
Copy link
Contributor

@shubhraagarwal Fixed the minor issues I found except for the test fixtures. Let me know if you want to pick that up.

@shubhraagarwal
Copy link
Author

shubhraagarwal commented Jan 8, 2025 via email

@alexbit-codemod
Copy link
Contributor

i still dont see the .ts file being added.... @shubhraagarwal once you make all the fixes let me know.

@alexbit-codemod
Copy link
Contributor

curious, can we close this out by the end of the week? @shubhraagarwal , @mohab-sameh ?

@shubhraagarwal
Copy link
Author

shubhraagarwal commented Jan 16, 2025 via email

@amirabbas-gh
Copy link
Collaborator

🚀 Hey @shubhraagarwal! What's up? 😃
We're eagerly waiting for your update! 🔄 What's the status of the issue? Has it been resolved? The Codemod community would love to see your improvements! 🚀🔥

@shubhraagarwal
Copy link
Author

shubhraagarwal commented Feb 8, 2025 via email

@shubhraagarwal
Copy link
Author

@amirabbas-gh @mohab-sameh have updated the testfixtures and tested it locally also, works good for me. Please let me know if any further changes are required

Copy link
Collaborator

@amirabbas-gh amirabbas-gh left a comment

Choose a reason for hiding this comment

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

I can't run tests using npm run test:

➜  rename-unsafe-lifecycles git:(rename-unsafe-lifecycles) ✗ npm run test

> test
> vitest run


 RUN  v1.6.1 /***/commons/codemods/react/rename-unsafe-lifecycles

 ❯ test/test.js (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/test.js [ test/test.js ]
ReferenceError: describe is not defined
 ❯ test/test.js:24:1
     22| const defineTest = require('jscodeshift/dist/testUtils').defineTest;
     23| 
     24| describe('rename-unsafe-lifecycles', () => {
       | ^
     25|   describe('flow', () => {
     26|     beforeEach(() => {

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  no tests
   Start at  22:25:04
   Duration  190ms (transform 42ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 70ms)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add **/test/*.js to this config because your tests are written in JavaScript.

@alexbit-codemod
Copy link
Contributor

cc @shubhraagarwal for the next steps...

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

Successfully merging this pull request may close these issues.

4 participants