feat(packages): eslint-plugin-turbo #1620
Conversation
tknickman
requested review from
jaredpalmer,
gsoltis,
gaspar09,
nathanhammond,
chris-olszewski and
mehulkar
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tknickman
force-pushed
the
eslint-plugin
branch
from
93f70b3
to
9409e92
Compare
packages/eslint-plugin-turbo/__tests__/lib/env-var-dependencies.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-turbo/docs/rules/no-uncached-env-vars.md
Outdated
Show resolved
Hide resolved
| { | ||
| "pipeline": { | ||
| "build": { | ||
| "dependsOn": ["^build", "$MY_API_TOKEN"], |
There was a problem hiding this comment.
Can we actually detect package vs. task vs. global?
There was a problem hiding this comment.
I did spend some time thinking about this, and I don't believe we could reliably infer this (without actually running the tasks)
There was a problem hiding this comment.
I made the comment because the correct patterns were in different locations and a tiny bit confusing. I don't think there is anything wrong with it; but it did make me pause.
There was a problem hiding this comment.
Small suggestion that may or may not conform to the existing workflow in the repo: It would be helpful to split the work that creates the utils package into a separate commit.
Other than that a meager review and mostly a bunch of questions! The meat of the implementation and tests look good to me other than @nathanhammond's suggestions!
| function findDependsOnEnvVars({ | ||
| dependencies, | ||
| }: { | ||
| dependencies?: Array<string>; |
There was a problem hiding this comment.
TS q... iiuc ? means that the argument is optional. Could we get by with assigning a default value to the arg (and therefore prevent the if check in the function body? (not a blocker, just curious!
There was a problem hiding this comment.
This is a transitive-to-user-input ?, so at some point it'll have to be optional. We can choose where we put the if statement, basically.
tknickman
force-pushed
the
eslint-plugin
branch
from
9409e92
to
5a77144
Compare
tknickman
force-pushed
the
eslint-plugin
branch
from
5a77144
to
a404d47
Compare
|
Addressed everything but the naming for now (still thinking about this one). Thanks for the reviews! |
tknickman
force-pushed
the
eslint-plugin
branch
from
a404d47
to
253511d
Compare
|
Name updated ( |
nathanhammond
added
the
pr: fixship
| ```json | ||
| { | ||
| "rules": { | ||
| "turbo/rule-name": "error" |
There was a problem hiding this comment.
| "turbo/rule-name": "error" | |
| "turbo/no-undeclared-env-vars": "error" |
| { | ||
| "name": "eslint-plugin-turbo", | ||
| "version": "0.0.1", | ||
| "description": "Static analysis for turbo", |
There was a problem hiding this comment.
Do you hold your pinky out when you drink your coffee?
| "description": "Static analysis for turbo", | |
| "description": "ESLint plugin for Turborepo", |
tknickman
force-pushed
the
eslint-plugin
branch
from
253511d
to
aa38df5
Compare
eslint-plugin-turboOnly one rule for now: error when an env var is provided that is not present in the turbo config.
NOTE: I'm building
turbo-utilsright now, because I want to tree shake it. But, I'm still exploring options for this with a pure internal library.Fixes TURBO-201