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

chore: Add support new types from React 18 #113

Merged
merged 6 commits into from
Jun 10, 2022
Merged

Conversation

filipewl
Copy link
Contributor

How does it work?

I had tried using a new feature from React 18 called useDeferredValue but got a type error Module '"react"' has no exported member 'useDeferredValue'.. I started by updating the type definitions from 17.x to 18.x but got another type error with HelmetProvider. I was able to remove it as it was not really needed, but the build failed again in the next type error in ErrorBoundary. Finally, after adjusting the missing type, I got everything working and with support for React 18 types.

How to test it?

Try to use any new React 18 features and their types should not fail to compile the build.

Filipe W. Lima added 3 commits June 10, 2022 06:53
https://github.com/vtex-sites/nextjs.store/runs/6822250131
```
info - Checking validity of types...
Failed to compile.

./src/components/sections/Hero/Hero.stories.tsx:19:4
Type error: No overload matches this call.
Overload 1 of 2, '(props: ProviderProps | Readonly): HelmetProvider', gave the following error.
Type '{ children: Element; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes & Readonly'.
Overload 2 of 2, '(props: ProviderProps, context: any): HelmetProvider', gave the following error.
Type '{ children: Element; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes & Readonly'.
```

This provider was being used in a storie just because Gatsby's version needs it and we were keeping the files in sync. But for some reason, after upgrading type definitions for React 18, this became a problem. I think it's safe to remove it since this is not needed for the NextJS version (the `Hero` storie still working proves that).
https://github.com/vtex-sites/nextjs.store/runs/6822439208
```
info - Checking validity of types...
Failed to compile.

./src/sdk/error/ErrorBoundary/ErrorBoundary.tsx:63:23
Type error: Property 'children' does not exist on type 'Readonly<{}>'.
```
@filipewl filipewl added the Chore General tasks. label Jun 10, 2022
@filipewl filipewl self-assigned this Jun 10, 2022
@vercel
Copy link

vercel bot commented Jun 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview Jun 10, 2022 at 3:04PM (UTC)

@vtex-sites
Copy link

vtex-sites bot commented Jun 10, 2022

Preview is ready

This pull request generated a Preview

👀   Preview: https://sfj-943421a--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 943421a

@vercel vercel bot temporarily deployed to preview June 10, 2022 10:32 Inactive
@filipewl filipewl marked this pull request as ready for review June 10, 2022 10:32
@filipewl filipewl requested a review from a team June 10, 2022 10:33
@vtex-sites
Copy link

vtex-sites bot commented Jun 10, 2022

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 943421a

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse-99988212/p
📎   /office

CHANGELOG.md Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to preview June 10, 2022 13:50 Inactive
@@ -93,7 +93,7 @@
"webpack": "^5.72.0"
},
"resolutions": {
"@types/react": "^17.x"
"@types/react": "^18.x"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the resolutions? Are we using more than one version of React?

Copy link
Member

@eduardoformiga eduardoformiga Jun 10, 2022

Choose a reason for hiding this comment

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

I think that's because the dependencies tree depends on different versions of @types/react and we can indicate the correct version using this resolutions option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This resolutions field is also new to me, it seems to be yarn-specific (not npm). I found some documentation here: [yarn] Selective dependency resolutions.

Curiously we don't have it for Gatsby. Also there I was able to have TS understand the new React 18 hooks by just adding this configuration to tsconfig.json, but that didn't work here in NextJS.
😞

@vercel vercel bot temporarily deployed to preview June 10, 2022 15:04 Inactive
@filipewl filipewl merged commit 943421a into main Jun 10, 2022
@filipewl filipewl deleted the chore/react18-support branch June 10, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore General tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants