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

codemod: add transform replaceReactUseRef #137

Closed
wants to merge 40 commits into from

Conversation

milahu
Copy link

@milahu milahu commented Nov 3, 2022

note: looking back, react2solid should be implemented as an eslint plugin
for example https://github.com/milahu/eslint-plugin-react2solid
why? eslint is the most popular transformer = large ecosystem = reuse existing rules
typescript is slow (except we use swc), typescript should be optional

transform React.useRef

-const divRef = React.useRef<HTMLDivElement>(null)
+var divRef: HTMLDivElement | null = null;
const div = <div ref={divRef}>asdf</div>
-console.log(divRef.current)
+console.log(divRef)

not perfect but should work for most cases

also

fix #133
fix #136

fix #135 (partial) - use toMatchInlineSnapshot to allow npx jest -u

@milahu milahu force-pushed the add-transform-replaceReactUseRef branch 6 times, most recently from 435de75 to 4a893d0 Compare November 3, 2022 15:41
@milahu milahu marked this pull request as ready for review November 3, 2022 15:42
@milahu milahu changed the title add transform replaceReactUseRef codemod: add transform replaceReactUseRef Nov 3, 2022
@milahu

This comment was marked as spam.

@juanrgm
Copy link
Member

juanrgm commented Nov 16, 2022

Thanks you for the contribution to the project!

Some issues to resolve:

  1. The current mainstream branch is develop.
  2. jest was replaced by vitest in the develop branch.
  3. There is no changeset file.
  4. There are a lot of changes in this PR. This can be a problem for the stability of the proyect because I can't to review all and the changelog will be a bunch of a lot of things (fix, feat, refactor).

Again, thanks for the contribution.

@milahu milahu force-pushed the add-transform-replaceReactUseRef branch from f78526d to f7c1dbf Compare November 16, 2022 15:32
@milahu milahu changed the base branch from main to develop November 16, 2022 15:32
@milahu
Copy link
Author

milahu commented Nov 16, 2022

4. There are a lot of changes in this PR. This can be a problem for the stability of the proyect because I can't to review all and the changelog will be a bunch of a lot of things (fix, feat, refactor).

thats why we have tests : )
old tests are passing, new features are covered by new tests ...

i am aware that this codemod was made to transform react-material-ui
but its also useful to transform other react components

but this is still a draft (acfba1f etc), not ready to merge

@milahu milahu marked this pull request as draft November 16, 2022 15:53
@milahu
Copy link
Author

milahu commented Nov 17, 2022

fix imports for node 19

node 19 has removed the --experimental-specifier-resolution flag
https://nodejs.org/nl/blog/announcements/v19-release-announce/#custom-esm-resolution-adjustments

so currently bin.ts throws

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'lib/actions/fixEsm' imported from lib/bin.js

imo, custom loaders are even worse than the experimental flag
and its simpler to add .js file extensions to all imports = 9b904ec

@juanrgm
Copy link
Member

juanrgm commented Nov 18, 2022

  1. There are a lot of changes in this PR. This can be a problem for the stability of the proyect because I can't to review all and the changelog will be a bunch of a lot of things (fix, feat, refactor).

thats why we have tests : ) old tests are passing, new features are covered by new tests ...

i am aware that this codemod was made to transform react-material-ui but its also useful to transform other react components

but this is still a draft (acfba1f etc), not ready to merge

True, but stability is not only the testing phase. Create a pull request with a lot of differents commits is not good:

  • Bug fixes are locked.
  • Semver is not continuous.
  • Collaboration with other users/developers is harder: only one PR (channel of communication) but there is a lot of issues, the discussion is very difficult.
  • The review by other developers is impossible because there is not a goal, anything goes.
  • The maintenance by the author of the PR will be hard when the base branch be updated.
  • Git conflicts are increased.
  • Posibility of introduce a bad feature coveraged by test is increased.
  • Locating the possible new bugs is more diffcult.
  • Reversion in case of problems is harder.
  • ...

I really appreciate your dedication, and your work looks great, but I need your help to keep the project in order with this pull request.

@milahu
Copy link
Author

milahu commented Nov 18, 2022

Create a pull request with a lot of differents commits is not good

so should i create many PRs?
or add one changeset per commit?

semver is overrated, npm is overrated ...
when people find bugs, they fetch the git history and bisect commits

@juanrgm
Copy link
Member

juanrgm commented Nov 19, 2022

The commits should be grouped by goals in multiple pull requests.

The bug fixes have priority and they should separated. If you wrote a test, first add the test commit, and later the fix commit. With this way, the reviewer can run the test without the fix.

The tests increment their complexity slowly. Only one complex test is a bad pattern. The complex test is needed, but in last instance.

If a new feat/fix requires structuring a lot of code, first try do a refactor commit and later the feat/fix commit. This will hep a lot of in the review.

The global changes must be debated with a issue and overall never mixed with others changes.

  • Should we add support to Node.js 19?
  • Should we use test-snapshot? Are required? It is this a anti-pattern?
  • Should we always set the develop branch as mainstream for the developing?
  • Should we add the patch-package dependency? pnpm already has a patch system.

These last issues are global changes and could be implemented later, without blocking other issues more importants.

Each PR must include the changeset file to increment the npm package version. This is only necessary if the changes affect to the end user of if this changes are used by other internal packages. A PR can't include several changeset files.

If a new react transformer or material component is added it is required regenerate the roadmap file (https://github.com/swordev/suid/blob/main/scripts/actions/genRoadmap.ts).

I know that this may seem complex, but it's very easy. Structuring the commits like this, the 80% of your commits would be merged already.

@milahu
Copy link
Author

milahu commented Nov 19, 2022

... right. i guess this also belongs to contributing.md

closing as "i dont need this any more"

edit: "... and im too lazy to get this merged"

@milahu milahu closed this Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants