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.

Sign up for GitHub

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

Jump to bottom

Added react imports to typescript examples #12018

Closed
wants to merge 1 commit into from
Closed

Added react imports to typescript examples #12018

wants to merge 1 commit into from

Conversation

viatsko
Copy link

@viatsko viatsko commented Apr 19, 2020 •
edited
Loading

Problem:

  • In normal usage it's just a linting error using default sane eslint/tslint configuration: 'React' must be in scope when using JSX using eslint-typescript defaults

  • Might cause issues with exotic typescript/webpack configurations which would prevent build (default next.js configuration)

  • (Minor) Coding-style persistency across examples

Reproduction:

  • Create new next-app using with-typescript example
yarn create next-app (pick with-typescript example)
  • Add .eslintrc.js to your project
module.exports = {
  parser: "@typescript-eslint/parser",
  extends: [
    "plugin:react/recommended",
    "plugin:@typescript-eslint/recommended",
  ],
  parserOptions: {
    ecmaVersion: 2018,
    sourceType: "module",
    ecmaFeatures: {
      jsx: true,
    },
  },
  settings: {
    react: {
      version: "detect",
    },
  },
};
  • Install dependencies for that config:
yarn add --dev @typescript-eslint/eslint-plugin @typescript-eslint/parser eslint eslint-plugin-react

  • Go to pages/index.tsx

  • You'll see an error in red if you use an editor with ESLint enabled, e. g. VSCode with ESLint plugin, or you'll get in error in the console by manually running eslint ./node_modules/bin/eslint pages/index.tsx

Screen Shot 2020-04-19 at 9 38 16 AM

@viatsko
Added react import to typescript examples
593dc5b 
Problem:

* Normal usage - linting error: 'React' must be in scope when using JSX using eslint-typescript defaults

* Might cause issues with exotic typescript/webpack configurations which would prevent build

* (Minor) Coding-style persistency across examples

Reproduction:

* Create new next-app using with-typescript example

```
yarn create next-app (pick with-typescript example)
```

* Add .eslintrc.js to your project

```
module.exports = {
  parser: "@typescript-eslint/parser",
  extends: [
    "plugin:react/recommended",
    "plugin:@typescript-eslint/recommended",
  ],
  parserOptions: {
    ecmaVersion: 2018,
    sourceType: "module",
    ecmaFeatures: {
      jsx: true,
    },
  },
  settings: {
    react: {
      version: "detect",
    },
  },
};
```

* Install dependencies for that config:

```
yarn add --dev @typescript-eslint/eslint-plugin @typescript-eslint/parser eslint eslint-plugin-react
```

* Go to `pages/index.tsx`

* You'll see error in red if you use an editor with ESLint enabled, e. g. VSCode with ESLint plugin, or you'll get in error in console by manually running eslint `./node_modules/bin/eslint pages/index.tsx`
@ijjk
Copy link
Member

ijjk commented Apr 19, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General
zeit/next.js canary viatsko/next.js canary Change
buildDuration 13.3s 13.1s -148ms
nodeModulesSize 55.5 MB 55.5 MB ✓
Page Load Tests Overall decrease ⚠️
zeit/next.js canary viatsko/next.js canary Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.269 2.401 ⚠️ +0.13
/ avg req/sec 1101.74 1041.32 -60.42
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.258 1.294 ⚠️ +0.04
/error-in-render avg req/sec 1987.34 1931.56 -55.78
Client Bundles (main, webpack, commons)
zeit/next.js canary viatsko/next.js canary Change
main-HASH.js gzip 6.25 kB 6.25 kB ✓
webpack-HASH.js gzip 746 B 746 B ✓
de003c3a9d30..5e5e.js gzip 10.3 kB 10.3 kB ✓
framework.a1..NSE.txt gzip 220 B 220 B ✓
framework.a1..NSE.txt gzip 220 B 220 B ✓
framework.HASH.js gzip 39 kB 39 kB ✓
Overall change 56.7 kB 56.7 kB ✓
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary viatsko/next.js canary Change
main-HASH.module.js gzip 4.79 kB 4.79 kB ✓
webpack-HASH..dule.js gzip 746 B 746 B ✓
de003c3a9d30..dule.js gzip 6.85 kB 6.85 kB ✓
framework.HA..dule.js gzip 39 kB 39 kB ✓
Overall change 51.4 kB 51.4 kB ✓
Legacy Client Bundles (polyfills)
zeit/next.js canary viatsko/next.js canary Change
polyfills-HASH.js gzip 26.2 kB 26.2 kB ✓
Overall change 26.2 kB 26.2 kB ✓
Client Pages
zeit/next.js canary viatsko/next.js canary Change
_app.js gzip 1.23 kB 1.23 kB ✓
_error.js gzip 3.12 kB 3.12 kB ✓
hooks.js gzip 663 B 663 B ✓
index.js gzip 222 B 222 B ✓
link.js gzip 2.06 kB 2.06 kB ✓
routerDirect.js gzip 280 B 280 B ✓
withRouter.js gzip 278 B 278 B ✓
Overall change 7.85 kB 7.85 kB ✓
Client Pages Modern
zeit/next.js canary viatsko/next.js canary Change
_app.module.js gzip 598 B 598 B ✓
_error.module.js gzip 2.09 kB 2.09 kB ✓
hooks.module.js gzip 383 B 383 B ✓
index.module.js gzip 223 B 223 B ✓
link.module.js gzip 1.52 kB 1.52 kB ✓
routerDirect..dule.js gzip 279 B 279 B ✓
withRouter.m..dule.js gzip 278 B 278 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
zeit/next.js canary viatsko/next.js canary Change
_buildManifest.js gzip 61 B 61 B ✓
_buildManife..dule.js gzip 61 B 61 B ✓
Overall change 122 B 122 B ✓
Rendered Page Sizes
zeit/next.js canary viatsko/next.js canary Change
index.html gzip 928 B 928 B ✓
link.html gzip 938 B 938 B ✓
withRouter.html gzip 926 B 926 B ✓
Overall change 2.79 kB 2.79 kB ✓
Serverless Mode
General
zeit/next.js canary viatsko/next.js canary Change
buildDuration 13.8s 14s ⚠️ +147ms
nodeModulesSize 55.5 MB 55.5 MB ✓
Client Bundles (main, webpack, commons)
zeit/next.js canary viatsko/next.js canary Change
main-HASH.js gzip 6.25 kB 6.25 kB ✓
webpack-HASH.js gzip 746 B 746 B ✓
de003c3a9d30..5e5e.js gzip 10.3 kB 10.3 kB ✓
framework.a1..NSE.txt gzip 220 B 220 B ✓
framework.a1..NSE.txt gzip 220 B 220 B ✓
framework.HASH.js gzip 39 kB 39 kB ✓
Overall change 56.7 kB 56.7 kB ✓
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary viatsko/next.js canary Change
main-HASH.module.js gzip 4.79 kB 4.79 kB ✓
webpack-HASH..dule.js gzip 746 B 746 B ✓
de003c3a9d30..dule.js gzip 6.85 kB 6.85 kB ✓
framework.HA..dule.js gzip 39 kB 39 kB ✓
Overall change 51.4 kB 51.4 kB ✓
Legacy Client Bundles (polyfills)
zeit/next.js canary viatsko/next.js canary Change
polyfills-HASH.js gzip 26.2 kB 26.2 kB ✓
Overall change 26.2 kB 26.2 kB ✓
Client Pages
zeit/next.js canary viatsko/next.js canary Change
_app.js gzip 1.23 kB 1.23 kB ✓
_error.js gzip 3.12 kB 3.12 kB ✓
hooks.js gzip 663 B 663 B ✓
index.js gzip 222 B 222 B ✓
link.js gzip 2.06 kB 2.06 kB ✓
routerDirect.js gzip 280 B 280 B ✓
withRouter.js gzip 278 B 278 B ✓
Overall change 7.85 kB 7.85 kB ✓
Client Pages Modern
zeit/next.js canary viatsko/next.js canary Change
_app.module.js gzip 598 B 598 B ✓
_error.module.js gzip 2.09 kB 2.09 kB ✓
hooks.module.js gzip 383 B 383 B ✓
index.module.js gzip 223 B 223 B ✓
link.module.js gzip 1.52 kB 1.52 kB ✓
routerDirect..dule.js gzip 279 B 279 B ✓
withRouter.m..dule.js gzip 278 B 278 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
zeit/next.js canary viatsko/next.js canary Change
_buildManifest.js gzip 61 B 61 B ✓
_buildManife..dule.js gzip 61 B 61 B ✓
Overall change 122 B 122 B ✓
Serverless bundles
zeit/next.js canary viatsko/next.js canary Change
_error.js 558 kB 558 kB ✓
404.html 4.18 kB 4.18 kB ✓
hooks.html 3.81 kB 3.81 kB ✓
index.js 558 kB 558 kB ✓
link.js 594 kB 594 kB ✓
routerDirect.js 587 kB 587 kB ✓
withRouter.js 587 kB 587 kB ✓
Overall change 2.89 MB 2.89 MB ✓

@matamatanot
Copy link
Contributor

ref: #8350 #11731

A plugin dedicated to Next.js will be released soon, and I don't feel the need to increase the amount of code to keep up with default rules.

@Timer
Copy link
Member

Timer commented May 2, 2020

Closing per above: importing React is not necessary in Next.js, and not doing so produces more efficient code. Please disable your lint rule!

@Timer Timer closed this May 2, 2020
@viatsko
Copy link
Author

viatsko commented May 3, 2020

The point was also about consistency. If you don't want React to be imported - let's not import it in all the files of the example (e. g. about has it, index is not). It's just confusing for beginners to understand what you really want

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lfades lfades Awaiting requested review from lfades

@Timer Timer Awaiting requested review from Timer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@viatsko @ijjk @matamatanot @Timer