javascript icon indicating copy to clipboard operation
javascript copied to clipboard

chore(clerk-js): React18 for clerk-js and shared

Open dvargas92495 opened this issue 3 years ago • 9 comments

Type of change

  • [ ] 🐛 Bug fix
  • [ ] 🌟 New feature
  • [ ] 🔨 Breaking change
  • [x] 📖 Refactoring / dependency upgrade / documentation
  • [x] other:

Packages affected

  • [x] @clerk/clerk-js
  • [ ] @clerk/clerk-react
  • [ ] @clerk/nextjs
  • [ ] @clerk/remix
  • [ ] @clerk/types
  • [ ] @clerk/clerk-expo
  • [x] @clerk/backend-core
  • [ ] @clerk/clerk-sdk-node
  • [x] @clerk/edge
  • [x] build/tooling/chore

Description

  • [ ] npm test runs as expected.
    • Issues listed below
  • [x] npm run build runs as expected.

I started work on https://github.com/clerkinc/javascript/issues/220 - ran into a few issues running clerk build/tests locally so I wanted to make a PR for it to prompt further discussion.

Issues:

  • Engine requirements - I'm currently have to run Node14 locally because AWS Lambda doesn't yet support 16, though they plan to later this month. However, if there's nothing specifically preventing being run on node14, might as well drop the requirement to 14 or drop it all together.
  • TURBO_CONCURRENCY - I did not have this env defined. It should either be defined for the developer by default or listed in the setup instructions
  • ./moduleTypeFix - converted these files to node scripts since windows doesn't support running ./ commands. but everyone running this package will have node
  • React test failures - This is the biggest issue I need help on - A ton of tests are failing for me locally due to Cannot read property 'useState' of null; basically React is null throughout the tests. I'm not sure exactly why this is or if this is related to bumping up to react 18 yet. React testing library does not yet support React 18 from what I've read, but I don't think that should imply React resolving to null.

After fixing the above, clerk-js was able to build correctly on React 18. Testing on React 18 is blocked by the test failures I mentioned above

dvargas92495 avatar May 05 '22 18:05 dvargas92495

tagging @nikosdouvlis for preemptive review and question surrounding React being null

dvargas92495 avatar May 05 '22 19:05 dvargas92495

Ah reading the CI failure, I'm seeing "Invalid hook call" which usually happens with multiple react versions. I think I know where to debug from here

dvargas92495 avatar May 05 '22 19:05 dvargas92495

@dvargas92495 some tests are still failing. Could you please have another look?

SokratisVidros avatar May 12 '22 12:05 SokratisVidros

yup, wasn't done iterating - I'll ping again when it's ready for review

dvargas92495 avatar May 12 '22 14:05 dvargas92495

Ok all tests are passing, I think the PR is officially ready for review! Taking out of draft

It's a big one, so I'm happy to split this out into smaller PRs if that would make things easier to merge. Package-lock in particular is scary so I would first do all the test changes as one PR, then the package upgrades as the second.

ccing: @nikosdouvlis + @SokratisVidros for review

I know according to #220, we also wanted to do some manual testing - how do you guys typically inspect package changes in the browser locally? There's no Storybook so figured there might be something in the repo that helps spin up the changes for development

Thanks @dvargas92495. We have tests applications locally for all ClerkJS and Clerk React components. These applications will be publicly available soon.

SokratisVidros avatar May 16 '22 11:05 SokratisVidros

@dvargas92495 Did you notice an increased duration in tests due to the addition of all the await before each user interaction? What's the official recommendation by https://testing-library.com/ for React 18?

SokratisVidros avatar May 16 '22 11:05 SokratisVidros

Comparing to main - it appears test times went up by a minute.

There is no mention of React 18 specifically on testinglibrary.com, but their latest docs recommend using await:

image

They also recommend using userEvent.setup() to invoke on new instances of userEvent instead of the default export. Happy to make this migration, but was thinking of doing that on a followup PR to minimize the surface area of this one. From the docs:

image

dvargas92495 avatar May 16 '22 15:05 dvargas92495

Just rebased to fix the new failing test

dvargas92495 avatar May 17 '22 22:05 dvargas92495

Hey, don't have permission to merge - just rebased again and made sure tests are passing

dvargas92495 avatar May 31 '22 16:05 dvargas92495