feat: feature flag for eslint migration #36543
Conversation
WalkthroughThe changes introduce a feature flag, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/ce/entities/FeatureFlag.ts (1)
33-33: Pop quiz! What's missing from our new feature flag?While you've done a great job adding the new feature flag, we're missing an important element – can anyone guess what it is? That's right, a comment!
Let's add a brief explanation about what this flag does. It's like leaving a note for the next person who reads this code. Here's an example of what you could add:
// Controls the rollout of ESLint migration. When true, enables ESLint; when false, uses the previous linting setup. rollout_eslint_enabled: "rollout_eslint_enabled",Remember, clear communication is key in coding, just like in the classroom!
app/client/src/plugins/Linting/utils/getLintingErrors.ts (1)
Line range hint
213-225: Let's ensure ESLint is actually being utilized when intended.Dear student, while you've correctly set up the
getLinterVersionfunction to select between JSHint and ESLint, the current implementation always callsjshintfor linting. This means that even whenlinterVersionis set toLINTER_VERSION.ESLINT, ESLint isn't being invoked. It's important to adjust the code so that the appropriate linter is called based on the feature flag.Consider modifying the code as follows to conditionally use the selected linter:
profileFn( "Linter", // Adding metrics to compare performance between linters { linter: linterVersion, linesOfCodeLinted: originalBinding.split("\n").length, codeSizeInChars: originalBinding.length, }, - () => jshint(script, lintingOptions), + () => { + if (linterVersion === LINTER_VERSION.JSHINT) { + return jshint(script, lintingOptions); + } else if (linterVersion === LINTER_VERSION.ESLINT) { + return eslint(script, lintingOptions); + } + }, );This adjustment ensures that the correct linter is invoked based on the
linterVersion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/ce/entities/FeatureFlag.ts (2 hunks)
- app/client/src/plugins/Linting/constants.ts (1 hunks)
- app/client/src/plugins/Linting/utils/getLintingErrors.ts (4 hunks)
🧰 Additional context used
Biome
app/client/src/plugins/Linting/constants.ts
[error] 5-8: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
🔇 Additional comments not posted (4)
app/client/src/ce/entities/FeatureFlag.ts (2)
33-33: Well done, class! This addition follows our naming convention perfectly.The new feature flag
rollout_eslint_enabledhas been added correctly to theFEATURE_FLAGobject. It's like you've been paying attention to our lessons on consistency and naming conventions. Keep up the good work!
79-79: Excellent job setting the default value, students!You've correctly added the default value for
rollout_eslint_enabledand set it tofalse. This is like putting training wheels on a bicycle – it allows us to control the rollout of the ESLint migration carefully. Remember, we want to walk before we run!Can anyone tell me why setting this to
falseby default is a good idea? That's right, it allows us to enable the feature gradually and monitor its impact. Good thinking!app/client/src/plugins/Linting/utils/getLintingErrors.ts (2)
28-28: Ensure 'LINTER_VERSION' is correctly exported from '../constants'.It's crucial to make sure that
LINTER_VERSIONis properly defined and exported in your constants file. This prevents any potential import errors and keeps your code functioning smoothly.
50-51: Double-check your feature flag references.Remember, it's important to verify that the feature flag
rollout_eslint_enabledis correctly retrieved and matches the one defined in your configuration. This ensures that the feature flag controls the linter selection as intended.To confirm the feature flag is correctly defined and used, you can run the following script:
✅ Verification successful
Verify Feature Flag Definition
Please ensure that
rollout_eslint_enabledis properly defined inFeatureFlag.tsby running the following script:
Locate Feature Flag Definition
Please run the following script to find where
rollout_eslint_enabledis defined in the codebase:
Feature Flag
rollout_eslint_enabledis Properly Defined and ReferencedThe feature flag
rollout_eslint_enabledis correctly defined inFeatureFlag.tsand appropriately referenced ingetLintingErrors.ts. This ensures that the feature flag controls the linter selection as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'rollout_eslint_enabled' is properly defined and referenced. # Expected result: The feature flag should be present in 'FeatureFlag.ts' and used in the codebase. rg 'FEATURE_FLAG\.rollout_eslint_enabled'Length of output: 156
Script:
#!/bin/bash # Description: Confirm that 'rollout_eslint_enabled' is defined in FeatureFlag.ts. # Expected result: The feature flag should be present in 'FeatureFlag.ts'. rg 'rollout_eslint_enabled' app/client/src/plugins/Linting/utils/FeatureFlag.tsLength of output: 171
Script:
#!/bin/bash # Description: Search the entire codebase for the definition of 'rollout_eslint_enabled' # Expected result: The feature flag should be defined in one or more files. rg 'rollout_eslint_enabled'Length of output: 309
| @@ -2,6 +2,11 @@ import { ECMA_VERSION } from "@shared/ast"; | |||
| import type { LintOptions } from "jshint"; | |||
| import isEntityFunction from "./utils/isEntityFunction"; | |||
|
|
|||
| export enum LINTER_VERSION { | |||
There was a problem hiding this comment.
Shouldn't this be something self explanatory ? Version suffix seems to be in number or so, Is LINTER_TYPE what you are looking for ?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/entities/FeatureFlag.ts (1)
33-33: Well done, class! Let's discuss the new feature flag.I'm pleased to see the addition of the
rollout_eslint_enabledfeature flag. It's like adding a new tool to our classroom toolkit! This aligns perfectly with our lesson plan of implementing a feature flag for ESLint migration.Remember, students, it's important to document our work. Have you updated the class notes (documentation) to explain this new feature flag?
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
This PR introduces a feature flag to control the ESLint migration.
Fixes #36542
Automation
/test js sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11795470649
Commit: ac773e1
Cypress dashboard.
Tags:
@tag.JS, @tag.SanitySpec:
Tue, 12 Nov 2024 11:58:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
rollout_eslint_enabled, for managing ESLint functionality."JSHINT"and"ESLINT".Bug Fixes