-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[WIP] Fix Framework Inference for eslint-config-turbo. #8505
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
🟢 CI successful 🟢Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these since the intent is to auto-include these.
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
@@ -81,6 +83,10 @@ function create(context: RuleContextWithOptions): Rule.RuleListener { | |||
} | |||
}); | |||
|
|||
frameworkInferencePrefixes.forEach((prefix) => { | |||
regexAllowList.push(new RegExp(`^${prefix}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be too naive, since it doesn't account for package boundaries. It's going to auto-allow NEXT_PUBLIC
in a Vite application, for example.
(In practice, this probably isn't a big problem?)
✅ This change can build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Let's chat about this today. I think we should compare this with what is available in vercel/vercel
. Ultimately we have to match turbo's inference here, but I think that was ported from vercel/vercel
originally. So instead of porting that back to JS I would prefer that we just use that inference directly.
But if we've diverged we'll need to roll it ourselves so we match directly. We'll also need to think about a good way to ensure this doesn't drift from what's in turbo core.
Description
A fairly naive implementation of Framework Inference (as it stands at the moment).
turbo
's Framework Inference support is per-package whereas this one wholesale allowslists all framework environment variables in every package.I tried my hand at making it per-package but fell flat. Could use some assistance if we don't think this is sufficient. From my perspective, the risk of someone using the wrong vars in the wrong framework is minimal and this is an improvement on our current support (which is none at all).
Testing Instructions
Double-checking the tests I have would be great. I've never written an ESLint plugin before and had...quite a time with this.