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

New Lint rule to restrict process env usage #3220

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fichtnerma
Copy link
Contributor

@fichtnerma fichtnerma commented Jan 20, 2025

Description

Added new eslint rules to restrict usage of process.env

  • Restrict using NEXT_PUBLIC_ variables
  • Restrict using other env variables than NODE_ENV in next.config.js
  • Restrict using process.env in *.loader.ts files

Acceptance criteria

Further information

},
},
{
files: ["*.loader.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

@fraxachun loader.ts is just a convention: the file can have any name or the loader function can be defined in the component file

do you think this catches a common error, although not 100%?

@@ -1,4 +1,5 @@
/* eslint-disable no-console */
/* eslint-disable no-restricted-syntax */
Copy link
Member

Choose a reason for hiding this comment

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

for site-pages this rule should be disabled for the whole site. And you should mention in the changeset (or even migration guide) that for projects using the old build that this rule should also be disabled there.

Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

IMO we shouldn't add this to the package at all, but make it a Demo/Starter-only change

Reasons:

  1. It's highly specific for our deployment setup
  2. The file name patterns will change often (e.g., it's next.config.mjs in the Starter, and will be next.config.ts starting with Next 15)
  3. There's no simple way to disable just this rule, devs would have to disable "no-restricted-syntax" altogether. This is okay for now, but once we add another rule, it will be disabled as a side effect.

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

Successfully merging this pull request may close these issues.

3 participants