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

Redundant React Imports in next.js/examples #12964

Closed
awareness481 opened this issue May 16, 2020 · 12 comments · Fixed by #15144
Closed

Redundant React Imports in next.js/examples #12964

awareness481 opened this issue May 16, 2020 · 12 comments · Fixed by #15144
Labels
good first issue Easy to fix issues, good for newcomers
Milestone

Comments

@awareness481
Copy link
Contributor

Feature request

Is your feature request related to a problem? Please describe.

Currently most examples in the examples folder include react imports in files under pages which is not needed. I can't recall if it ever was needed in Next.js (Perhaps some examples with an older next.js version need the imports)

Describe the solution you'd like

Remove react imports from files that don't need. Method of testing is up to you to decide but I presume running the project, checking for console errors and making sure the functionality is the same should be sufficient?

Alternatives Considering whether the changes are worth it

Is this something that the Next.js team think should be changed? It's perfectly reasonable if you guys think this isn't worth the effort. With that said, I would love the honor of contributing to the Next.js repo, even with something that is probably considered a chore.

I don't know if there are any technical differences with redundantly importing React but it could be argued that's it's bad practice.

Additional context

Some components are written with classes where as far as I can tell React imports are needed.

import React, { PureComponent } from 'react'
import { connect } from 'react-redux'
import Link from 'next/link'
import { startClock, serverRenderClock } from '../actions'
import Examples from '../components/examples'

class Index extends PureComponent {
//...
}

Should those be updated to functional components or be left alone?

I also forked the repo just to showcase what the differences would be using the with-redux and with-redux-thunk examples. with-redux-thunk uses a class component in pages/index.js so there I didn't remove the React import.

Git diff

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels May 16, 2020
@timneutkens
Copy link
Member

Next.js automatically adds the React import when JSX is used indeed. However keep in mind that we do still need to import React from 'react' when the React variable is used. Besides that yeah it seems like a good one to contribute 👍 Marked as good first issue.

@mattcarlotta
Copy link
Contributor

mattcarlotta commented May 16, 2020

My fault for the redundant imports in that example above. I've submitted a PR to remove them from the with-redux-thunk example. That said, there's definitely some missing documentation about React being automatically imported within JSX files.

On that note, since it's automatically imported, would it be more efficient to provide React as a global in development (that way, default React exports, like React.Component, don't need to be reimported)? Understandably, it still wouldn't make sense to provide the named exports nor to have React be a global in production.

@teo-garcia
Copy link
Contributor

Hello @timneutkens, is it ok to send a PR for each example? I'm working on a list with those examples that use a redundant React import

Maybe we could list them in this issue and mark the solved ones

@timneutkens
Copy link
Member

My fault for the redundant imports in that example above. I've submitted a PR to remove them from the with-redux-thunk example. That said, there's definitely some missing documentation about React being automatically imported within JSX files.

On that note, since it's automatically imported, would it be more efficient to provide React as a global in development (that way, default React exports, like React.Component, don't need to be reimported)? Understandably, it still wouldn't make sense to provide the named exports nor to have React be a global in production.

We've thought about it but it makes optimizing harder later on, e.g. when esmodules support is in a high percentage of browsers.

@teo-garcia
Copy link
Contributor

teo-garcia commented May 18, 2020

Is it possible to avoid the implicit import if there's a React instance or maybe warn the user when import the namespace? Has this situation any impact on the size of the bundle?

kodiakhq bot pushed a commit that referenced this issue May 20, 2020
This issue is related to [12694](#12964). I covered the following examples


- with-zeit-fetch
- with-yarn-workspaces
- with-why-did-your-render
- with-video-js
- with-universal-configuration-runtime
- with-typestyle
- with-three-js

If you have a suggestion or change I'd appreciate it
kodiakhq bot pushed a commit that referenced this issue May 20, 2020
The issue is related to [12964](#12964)

Let me know if there are any changes you want me to make.

Affected examples:

**with-glamor
with-graphql-hooks
with-graphql-react
with-grommet
with-http2
with-jest
with-cookie-auth-fauna
with-context-api
with-cerebral
with-aphrodite
with-apollo-and-redux
basic-css
with-carbon-components
amp-first**

I would love to help more, so let me know if there is anything specific I can contribute to.
@willianjusten
Copy link
Contributor

willianjusten commented May 20, 2020

@mvfsillva and I were removing in some examples, we pushed on #13169 and he's going to take the rest after =)

chibicode pushed a commit to chibicode/next.js that referenced this issue May 21, 2020
This issue is related to [12694](vercel#12964). I covered the following examples


- with-zeit-fetch
- with-yarn-workspaces
- with-why-did-your-render
- with-video-js
- with-universal-configuration-runtime
- with-typestyle
- with-three-js

If you have a suggestion or change I'd appreciate it
chibicode pushed a commit to chibicode/next.js that referenced this issue May 21, 2020
The issue is related to [12964](vercel#12964)

Let me know if there are any changes you want me to make.

Affected examples:

**with-glamor
with-graphql-hooks
with-graphql-react
with-grommet
with-http2
with-jest
with-cookie-auth-fauna
with-context-api
with-cerebral
with-aphrodite
with-apollo-and-redux
basic-css
with-carbon-components
amp-first**

I would love to help more, so let me know if there is anything specific I can contribute to.
kodiakhq bot pushed a commit that referenced this issue May 22, 2020
Again, related to [12964](#12964)

After checking all the other examples and the ongoing pull requests, I believe that with this PR being merged, all the examples should be free of redundant react imports.
Let me know if you want me to edit anything that you don't like.

Regards

with-typescript
with-atstroturf
with-atlaskit
with-styletron
with-styled-components-rtl
with-stylesheet
with-stomp
with-stitches-styled
with-stitches
with-slate
with-sentry-simple
with-sentry
with-segment-analytics
with-rematch
with-relay-modern
with-reflux
with-redux-wrapper
with-react-relay-network
with-react-native
with-react-multi-carousel
with-react-jss
with-react-helmet
with-react-ga
with-quill-js
with-prefetching
with-google-analytics-amp
with-google-analytics
with-framer-motion
with-flow
with-firebase-hosting
with-firebase-cloud-messaging
with-firebase-authentication
with-expo
with-dynamic-app-layout
with-draft-js
with-cxs
with-cerebral
with-ant-design-mobile
with-algolia-react-instantsearch
using-preact
progressive-render
kodiakhq bot pushed a commit that referenced this issue May 22, 2020
This issue is related to #12964 

**I changed the following examples:**
`with-zeit-fetch` 
`with-why-did-you-render`
`with-styletron`
`custom-server-fastify`
`custom-server-express`
`with-why-did-you-render`
`custom-server-hapi`
`custom-server-koa`
`custom-server-polka`
`custom-server-typescript`
`progressive-render`
`ssr-caching`
`svg-components`
`using-preact`
`with-ant-design`
@awareness481
Copy link
Contributor Author

Assuming the last PR will be merged, the only remaining examples are:

  • with-linaria
  • with-overmind

For with-overmind I'm waiting to see whether PR #13385 will be accepted before I remove the react imports.

with-linaria $ npm run dev failed on my system but I didn't have time to investigate.

kodiakhq bot pushed a commit that referenced this issue May 26, 2020
Per #12964

* with-ant-design
* with-dynamic-import
* with-iron-session
* with-monaco-editor
* with-next-page-transitions
* with-react-with-styles
* with-style-sheet
* with-why-did-you-render

Tested each example, working as intended, no additional issues presented
kodiakhq bot pushed a commit that referenced this issue May 27, 2020
Per #12964

Removed redundant react imports from next.js/examples/with-overmind
@awareness481
Copy link
Contributor Author

awareness481 commented May 27, 2020

@timneutkens With PR #13422 the only remaining example is with-linaria, excluding class components.

I was unable to run with-linaria on my system so I opened an issue #13423 . I'll investigate further if nobody picks up the issue within a week.

P.S I felt it was relevant to tag you, please feel free to say if it bothered you.

@Timer
Copy link
Member

Timer commented May 27, 2020

Exciting work! Thanks to all involved.
FWIW, we get all notifications, so it's no biggy!

@Timer Timer removed the help wanted label Jun 3, 2020
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
This issue is related to [12694](vercel#12964). I covered the following examples


- with-zeit-fetch
- with-yarn-workspaces
- with-why-did-your-render
- with-video-js
- with-universal-configuration-runtime
- with-typestyle
- with-three-js

If you have a suggestion or change I'd appreciate it
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
The issue is related to [12964](vercel#12964)

Let me know if there are any changes you want me to make.

Affected examples:

**with-glamor
with-graphql-hooks
with-graphql-react
with-grommet
with-http2
with-jest
with-cookie-auth-fauna
with-context-api
with-cerebral
with-aphrodite
with-apollo-and-redux
basic-css
with-carbon-components
amp-first**

I would love to help more, so let me know if there is anything specific I can contribute to.
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
Again, related to [12964](vercel#12964)

After checking all the other examples and the ongoing pull requests, I believe that with this PR being merged, all the examples should be free of redundant react imports.
Let me know if you want me to edit anything that you don't like.

Regards

with-typescript
with-atstroturf
with-atlaskit
with-styletron
with-styled-components-rtl
with-stylesheet
with-stomp
with-stitches-styled
with-stitches
with-slate
with-sentry-simple
with-sentry
with-segment-analytics
with-rematch
with-relay-modern
with-reflux
with-redux-wrapper
with-react-relay-network
with-react-native
with-react-multi-carousel
with-react-jss
with-react-helmet
with-react-ga
with-quill-js
with-prefetching
with-google-analytics-amp
with-google-analytics
with-framer-motion
with-flow
with-firebase-hosting
with-firebase-cloud-messaging
with-firebase-authentication
with-expo
with-dynamic-app-layout
with-draft-js
with-cxs
with-cerebral
with-ant-design-mobile
with-algolia-react-instantsearch
using-preact
progressive-render
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
This issue is related to vercel#12964 

**I changed the following examples:**
`with-zeit-fetch` 
`with-why-did-you-render`
`with-styletron`
`custom-server-fastify`
`custom-server-express`
`with-why-did-you-render`
`custom-server-hapi`
`custom-server-koa`
`custom-server-polka`
`custom-server-typescript`
`progressive-render`
`ssr-caching`
`svg-components`
`using-preact`
`with-ant-design`
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
Per vercel#12964

* with-ant-design
* with-dynamic-import
* with-iron-session
* with-monaco-editor
* with-next-page-transitions
* with-react-with-styles
* with-style-sheet
* with-why-did-you-render

Tested each example, working as intended, no additional issues presented
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
Per vercel#12964

Removed redundant react imports from next.js/examples/with-overmind
@kodiakhq kodiakhq bot closed this as completed in #15144 Jul 14, 2020
kodiakhq bot pushed a commit that referenced this issue Jul 14, 2020
This should close #12964 but I'm leaving it up to one of the Tims (or if there are other maintainers) to close the issue as they see fit.
@Timer Timer added this to the iteration 5 milestone Jul 14, 2020
@nullhook
Copy link

nullhook commented Aug 18, 2020

Next.js automatically adds the React import when JSX is used indeed. However keep in mind that we do still need to import React from 'react' when the React variable is used. Besides that yeah it seems like a good one to contribute 👍 Marked as good first issue.

Is this still valid? In the latest version of nextjs I didn't import React but I still was able to use React.useState() without any issues.

@timneutkens
Copy link
Member

@nullhook you should always import hooks as it's non-standard that React is available.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants