这是indexloc提供的服务,不要输入任何密码
Skip to content

feat(eslint-plugin): add new configuration style #2041

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

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

iduuck
Copy link
Contributor

@iduuck iduuck commented Sep 22, 2022

As I saw a new version popping up through my renovate integration, I updated. Though, linting is failing, since we are using the eslint-plugin-turbo plugin, and this is not yet updated to aim at the new configuration style.

This PR aims to fix this.

@iduuck iduuck requested a review from a team as a code owner September 22, 2022 14:21
@vercel
Copy link

vercel bot commented Sep 22, 2022

@iduuck is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@tknickman tknickman self-requested a review September 22, 2022 14:26
@vercel
Copy link

vercel bot commented Sep 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
turbo-site ✅ Ready (Inspect) Visit Preview Sep 22, 2022 at 3:39PM (UTC)

const envVarSet = new Set(
allEnvVars.map((envVar) => envVar.slice(1, envVar.length))
allEnvVars.map((envVar) =>
envVar.startsWith("$") ? envVar.slice(1, envVar.length) : envVar
Copy link
Member

@tknickman tknickman Sep 22, 2022

Choose a reason for hiding this comment

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

We don't want to clean $ from vars in env or globalEnv (because an env name with a leading $ could be a valid env var and we want to respect that). So instead of cleaning here, let's remove this altogether and move the cleaning to findDependsOnEnvVars since this will only be called for dependsOn or globalDependencies. Then we can leave env and globalEnv as is.

@@ -9,6 +9,7 @@ const ruleTester = new RuleTester({
const getTestTurboConfig = () => {
return {
$schema: "./docs/public/schema.json",
globalEnv: ["NEW_STYLE_GLOBAL_ENV_KEY"],
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test case where we have an env var that is prefixed with $ here and in env below.

For example,

globalEnv: ["NEW_STYLE_GLOBAL_ENV_KEY", "$NEW_STYLE_GLOBAL_ENV_KEY"],

And then this should pass:

const val = process.env["$NEW_STYLE_GLOBAL_ENV_KEY"];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Didn't think of this :)

@tknickman
Copy link
Member

Thanks for the contribution @iduuck! A few quick comments, but overall looks 🔥. Thanks for being ahead of the game 😄

@iduuck
Copy link
Contributor Author

iduuck commented Sep 22, 2022

@tknickman Fixed this and implemented the missing $-prefixed environment variables. Thanks for the review!

Copy link
Member

@tknickman tknickman left a comment

Choose a reason for hiding this comment

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

Great, ty!

@tknickman tknickman added the pr: automerge Kodiak will merge these automatically after checks pass label Sep 22, 2022
@tknickman tknickman changed the title add new configuration style to eslint-plugin feat(eslint-plugin): add new configuration style Sep 22, 2022
@tknickman tknickman merged commit a782622 into vercel:main Sep 22, 2022
@iduuck iduuck deleted the feature/configuration-changes-eslint branch September 22, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: automerge Kodiak will merge these automatically after checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants