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(codemod): Add codemod for js to jsx conversion #8551

Merged
merged 10 commits into from
Jun 12, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Jun 7, 2023

Problem
We're moving to require(?) that js files containing JSX have the .jsx extension.

Description
This codemod looks through all the .js files inside of web/src and examines their content for any JSX. If JSX is found the file extension is changed to .jsx.

Note
I had to add some custom test helper code as the existing folder matching one didn't fit my needs. Happy to discuss and iterate on this.

Outstanding
Need to confirm we're happy with the following:

  1. The test example data - is it broad enough?
  2. The check logic - any JSXElement, JSXFragment or JSXText flag the file as containing JSX
  3. The new test helper stuff - we good with it? Might need some minor refactoring?

I had to add some custom test helpers as the existing folder matching one didn't fit my needs. Happy to discuss and iterate on this.
@Josh-Walker-GM Josh-Walker-GM added the release:feature This PR introduces a new feature label Jun 7, 2023
@Josh-Walker-GM Josh-Walker-GM requested a review from dac09 June 7, 2023 18:23
On windows we'd get the original file back because we were writing it again with the source we were returning. Slight refactor to make things nicer too.
@Josh-Walker-GM Josh-Walker-GM marked this pull request as ready for review June 7, 2023 22:09
@dac09
Copy link
Collaborator

dac09 commented Jun 9, 2023

Hey Josh, I think you could achieve the same tests without needing to run the test on a full src folder.

The advantage here is that the tests become more specific (and easier to follow). All you need to do is just take your fixtures and put them in input and output files!

Remember its the yargs file that determines which files the transform is run against, not the actual codemod. I think the new test util makes this a little unclear.

Otherwise looking ok!

@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented Jun 9, 2023

Hey @dac09, I agree that it does make the testing a little less clear on the specific mapping/expectation of input to output.

I'm more than happy to rework this to avoid introducing a less ideal template for codemod testing. I think the issue was that the existing test utils for files were solely testing file content and so wouldn't check the change to the extension I was primarily intending to test for.

Writing this I just thought that I could mock out the fs calls within the jest tests and test the renaming of the extension that way. This would still allow me to use the existing process of feeding in test source from an input file and expect a source output matching the corresponding output file. Although in this case if you look at the test fixtures they're just a bunch of identical input and output files.

Copy link
Collaborator

dac09 commented Jun 9, 2023

Yeah it is an unusual codemod, because its technically a “structure” codemod that uses jscodeshift.

I think mocking out the fs makes more sense here, exactly what you’re suggesting. Because I think we’re really looking to verify two things:

  1. is it detecting JSX in files (use existing util to feed in stuff)
  2. is it renaming correctly (use fs to validate)

I’m not a 100% sure if this makes things more complicated - but I like the idea! But if you feel the need to add another util, that’s totally OK too - we just need to find a good name to make it clear what it does!

This makes the new test util a little more appealing and applicable to future use cases. Also adds some new test fixture folders to show a different testing approach.
@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented Jun 9, 2023

Okay it wasn't really possible to reuse the matchTransformSnapshot or matchInlineTransformSnapshot here. Those helpers:

  • Copy the input file to a temp file
  • Run the codemod against the temp file
  • Check the temp file contents match the output file

This codemod renames the temp file as part of it's job so that means the output check is done against a non existing file. Edit: I guess it could have worked if I'd mocked out the fs calls but that's maybe a bad road to go down. Testing the codemods but also having to start mocking out the functionality. I don't really like that all that much.

I think what I had is actually what we should go for. I updated it to be a little more applicable to future codemods and nicer to use. I added some tests in the same spirit I think you were suggesting you'd prefer them to be. If you can take a look at these changes and give me your opinion if this is okay to stick around?

I knew the naming would never survive contact with anyone else 😆 Happy to rename it to anything you think fits.

Copy link
Collaborator

dac09 commented Jun 9, 2023

I’ll take a look in the morning Josh, thanks 🙂

Pretty sure you’ve got this covered, but might have some ideas when I look at it tomorrow. Cheers!

@dac09
Copy link
Collaborator

dac09 commented Jun 12, 2023

@Josh-Walker-GM I managed to update the matchFolderTransform test util to just take an option to run the transform with jscodeshift instead. Not ideal either, but I think it does reduce duplication of code a little (one less thing to learn when you're writing codemod tests)

Let me know what you think!

Copy link
Collaborator

@dac09 dac09 left a comment

Choose a reason for hiding this comment

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

LGTM - but let's wait for Josh to merge, because he may want to review the changes I've done!

@Josh-Walker-GM
Copy link
Collaborator Author

@dac09 This is better thanks! I just added a minor option to the test that has the full example project structure - passed in the glob. Going to enable auto-merge on this now.

@Josh-Walker-GM Josh-Walker-GM enabled auto-merge (squash) June 12, 2023 15:27
@Josh-Walker-GM Josh-Walker-GM merged commit ffaae8b into main Jun 12, 2023
25 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw-jsx-codemod branch June 12, 2023 15:49
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 12, 2023
@jtoar jtoar modified the milestones: next-release, v6.0.0 Jun 13, 2023
dac09 added a commit to dac09/redwood that referenced this pull request Jun 13, 2023
…into chore/rename-entry-client

* 'chore/rename-entry-client' of github.com:dac09/redwood:
  Move fastify.logger.infos to fastify.logger.traces (redwoodjs#8590)
  Make the fastify logger respect the LOG_LEVEL value set in the env (redwoodjs#8588)
  feat: Update the SDL types lib (redwoodjs#8586)
  Fix the codegen path for the GraphQL context objcet (redwoodjs#8585)
  chore(deps): update dependency @clerk/types to v3.42.0 (redwoodjs#8584)
  feat(codemod): Add codemod for js to jsx conversion (redwoodjs#8551)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants