Common Next.js issues that could use integrated ESLint rules #27051

leerob · 2021-07-09T16:28:34Z

Describe the feature you'd like to request

These are common issues developers run into with Next.js. Some are mentioned in the docs, but ideally you don't have to go check the docs.

Describe the solution you'd like

Instead, ESLint can provide compile-time feedback and suggestions.

Trying to call an API route from inside getStaticProps / getServerSideProps leading to next build failing.
- Also related error → "Only absolute URLs are supported on the server"
Trying to use getStaticProps inside of _app.js, assuming it works like the other pages, but it doesn't.

Forgetting to await an async function inside an API route, works fine locally but does out of band work which will cause issues if there’s multiple requests.

Code Example

// Found on stackoverflow
export default function handler(req, res) {
    const { method } = req;
    
    switch (method) {
        case 'GET':
	   // but this is making a fetch and has no await....
            getRestaurants();
            break;
        default:
            res.status(405).end(`Method ${req.method} Not Allowed`);
    }

    function getRestaurants() {
        const restaurants = data;
        res.status(200).json(restaurants);
    }
}

Using document or other browser APIs outside of useEffect
File name casing (e.g. myFile.js vs MyFile.js). This could potentially be solved with linting if import statement diffs from the actual file path? Not sure.

Describe alternatives you've considered

N/A

The text was updated successfully, but these errors were encountered:

stefanprobst · 2021-07-11T07:54:55Z

~~maybe also a rule for where global css imports are allowed?~~ Edit: probably not now, since this behavior will change soon afaiu.

Patil2099 · 2021-07-11T09:45:16Z

I would love to work on this @leerob. Can you please assign this to me?

leerob · 2021-07-12T01:27:12Z

@Patil2099 Which one specifically would you like to work on first?

tarokawada · 2021-07-13T14:08:14Z

@leerob I'll work on Using document or other browser APIs outside of useEffect

Just to confirm, are you looking to include these interfaces as well https://developer.mozilla.org/en-US/docs/Web/API#interfaces ?

JacobLey · 2021-07-13T14:09:23Z

Forgetting to await an async function inside an API route, works fine locally but does out of band work which will cause issues if there’s multiple requests.

I'm not sure how you were planning on detecting API/promises, but if it is typescript based it can probably either defer to this existing rule or extend it somehow if necessary:
https://github.com/typescript-eslint/typescript-eslint/blob/v4.28.3/packages/eslint-plugin/docs/rules/no-floating-promises.md

housseindjirdeh · 2021-07-14T17:34:41Z

Not sure if its a super-common issue, but an integrated ESLint rule to catch #27134 would be nice to have.

~~I'll submit a PR for this soon!~~ PR here: #27179

leerob · 2021-07-21T12:08:35Z

Another one: throw an error if using fs or other node packages on the client side.

https://twitter.com/leeerob/status/1417818534026022912?s=21

yordis · 2021-07-25T21:00:00Z

File name casing (e.g. myFile.js vs MyFile.js). This could potentially be solved with linting if import statement diffs from the actual file path? Not sure.

We enforce to be kebab-case file name, no more case sensitive vs not issues. Worth reading a bit more here: https://github.com/straw-hat-team/adr/tree/master/adrs/3122196229

I think the problem begins by adopting things that cause more harm than good in the long run with little to no value IMO. I don't remember anymore how many times I had to fix CI by telling people to fix the casing situation and reminds them to use git mv

I would discourage the usage of camelCase or PascalCase and stick to kebab-case as NodeJS used to be until we diverged.

:2cents:

leerob · 2021-07-29T20:23:15Z

@yordis I'm not saying Next.js ESLint should have a preference on how you do casing – that's a bit to stylistic to determine at the framework level. I'm talking about when you import a file MyFile.js but the actual file name is cased different, like myFile.js.

yordis · 2021-07-30T00:03:26Z

Oh like, check for those errors?! Sorry, I misunderstood

kyliau · 2021-08-05T17:51:03Z

Hello everyone! I'm Keen and I just joined Google's WebSDK (Aurora) team this week 😃
I'd like to tackle the third item in Lee's issue as a starter project:

Forgetting to await an async function inside an API route, works fine locally but does out of band work which will cause issues if there’s multiple requests.

Please let me know if you have any concerns, otherwise I'll get started!

LetItRock · 2021-08-05T22:40:41Z

hi all! @leerob I would like to try to implement this rule:
Another one: throw an error if using fs or other node packages on the client side.

Just to clarify, the rule should not allow using node built-in modules inside the react components, but will allow inside the other files like ex. utils, API routes and inside the data fetching functionsgetStaticProps, getStaticPaths, getServerSideProps is that correct?

This commit (partially) tackles item 3 in vercel#27051: > Forgetting to await an async function inside an API route, works fine > locally but does out of band work which will cause issues if there’s > multiple requests. The work here is focused on TypeScript for now because `typescript-eslint` already provides `no-floating-promises` rule that precisely targets this scenario. This requires adding a new dependency (`@typescript-eslint/eslint-plugin`) to `eslint-config-next`. Although it is a superfluous dependency for JavaScript projects, it's not a big deal since `eslint-config-next` already has a dependency on `@typescript-eslint/parser`.

stefanprobst · 2021-08-20T08:27:28Z

what do you think about a rule which suggests adding a key prop on meta elements? (like this)

leerob · 2021-08-20T15:42:03Z

@stefanprobst Yeah, I think that makes sense - only if there are duplicate elements right? If there's a single meta tag, you don't need a key, correct? I wonder if we could make it smart enough to warn only if there are duplicates.

stefanprobst · 2021-08-20T16:02:38Z

the docs on this are a bit sparse, but if i understand correctly the point is that it's hard to know if somewhere else in the tree the same meta property gets set, so it's best to use a key prop to avoid duplicate meta tags(??)

for example, if you have:

// pages/_app.js
import Head from "next/head";
export default function App({ Component, pageProps }) {
  return (
    <>
      <Head>
        <meta property="og:title" content="My app" />
      </Head>
      <Component {...pageProps} />
    </>
  );
}

and

// pages/index.js
import Head from "next/head";
export default function HomePage() {
  return (
    <>
      <Head>
        <meta property="og:title" content="Home page" />
      </Head>
      <main>
        <h1>Hello world</h1>
      </main>
    </>
  );
}

you will end up with both <meta property="og:title" /> tags in the head.

note: meta tags with name attribute are deduplicated automatically, so this is mostly about <meta property="..." /> afaiu

nguyen-michael · 2021-10-05T06:00:57Z

@leerob

Thoughts on a rule that lints if next/image is using layout= fill or responsive but isn't setting explicit sizes prop?

Docs state:

A string mapping media queries to device sizes. Defaults to 100vw. We recommend setting sizes when using layout="responsive" or layout="fill" and your image will not be the same width as the viewport.

The rule could be opted into as a warning and reminder to set proper sizes or risk loading images that are too large if they're not going to be 100vw.

I've got a working implementation of this rule in a custom ESLint plugin stored on a private repo but would happily put it up for review if you think it would be handy!

leerob added template: story good first issue labels Jul 9, 2021

timneutkens added kind: story and removed template: story labels Jul 10, 2021

LetItRock mentioned this issue Aug 7, 2021

ESLint rule - no server modules on client side #27847

Open

9 tasks

kyliau mentioned this issue Aug 17, 2021

add typescript-eslint to eslint-config-next to catch floating promises #28181

Closed

10 tasks

Aug	OCT	Nov
	08
2020	2021	2022

vercel / next.js Public

Common Next.js issues that could use integrated ESLint rules #27051

Common Next.js issues that could use integrated ESLint rules #27051

leerob commented Jul 9, 2021

stefanprobst commented Jul 11, 2021 •

edited

Patil2099 commented Jul 11, 2021

leerob commented Jul 12, 2021

tarokawada commented Jul 13, 2021

JacobLey commented Jul 13, 2021

housseindjirdeh commented Jul 14, 2021 •

edited

leerob commented Jul 21, 2021

yordis commented Jul 25, 2021 •

edited

leerob commented Jul 29, 2021

yordis commented Jul 30, 2021

kyliau commented Aug 5, 2021

LetItRock commented Aug 5, 2021

stefanprobst commented Aug 20, 2021

leerob commented Aug 20, 2021

stefanprobst commented Aug 20, 2021

nguyen-michael commented Oct 5, 2021

vercel / next.js Public

Common Next.js issues that could use integrated ESLint rules #27051

Common Next.js issues that could use integrated ESLint rules #27051

Comments

leerob commented Jul 9, 2021

Describe the feature you'd like to request

Describe the solution you'd like

Describe alternatives you've considered

stefanprobst commented Jul 11, 2021 • edited

Patil2099 commented Jul 11, 2021

leerob commented Jul 12, 2021

tarokawada commented Jul 13, 2021

JacobLey commented Jul 13, 2021

housseindjirdeh commented Jul 14, 2021 • edited

leerob commented Jul 21, 2021

yordis commented Jul 25, 2021 • edited

leerob commented Jul 29, 2021

yordis commented Jul 30, 2021

kyliau commented Aug 5, 2021

LetItRock commented Aug 5, 2021

stefanprobst commented Aug 20, 2021

leerob commented Aug 20, 2021

stefanprobst commented Aug 20, 2021

nguyen-michael commented Oct 5, 2021

stefanprobst commented Jul 11, 2021 •

edited

housseindjirdeh commented Jul 14, 2021 •

edited

yordis commented Jul 25, 2021 •

edited