close
The Wayback Machine - https://web.archive.org/web/20210813135431/https://github.com/vercel/next.js/issues/27051
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

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

Open
leerob opened this issue Jul 9, 2021 · 12 comments
Open

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

leerob opened this issue Jul 9, 2021 · 12 comments

Comments

@leerob
Copy link
Member

@leerob leerob commented Jul 9, 2021

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.

  • 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

@stefanprobst
Copy link
Contributor

@stefanprobst stefanprobst commented Jul 11, 2021

maybe also a rule for where global css imports are allowed?

@Patil2099
Copy link

@Patil2099 Patil2099 commented Jul 11, 2021

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

@leerob
Copy link
Member Author

@leerob leerob commented Jul 12, 2021

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

@tarokawada
Copy link

@tarokawada tarokawada commented Jul 13, 2021

@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
Copy link
Contributor

@JacobLey JacobLey commented Jul 13, 2021

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
Copy link
Collaborator

@housseindjirdeh housseindjirdeh commented Jul 14, 2021

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
Copy link
Member Author

@leerob leerob commented Jul 21, 2021

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
Copy link
Contributor

@yordis yordis commented Jul 25, 2021

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
Copy link
Member Author

@leerob leerob commented Jul 29, 2021

@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
Copy link
Contributor

@yordis yordis commented Jul 30, 2021

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

@kyliau
Copy link

@kyliau kyliau commented Aug 5, 2021

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
Copy link
Contributor

@LetItRock LetItRock commented Aug 5, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants