-
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
feat(upgrade): adds featureflag-deprecate-flags-prop codemod and deprecates flags prop #17266
feat(upgrade): adds featureflag-deprecate-flags-prop codemod and deprecates flags prop #17266
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @tay1orjones one doubt , what do you think on test fixture ? can we just make it generic for all feature flags , May be just this The input.js
The output.js ;` |
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 looks great! Just a few things from our pair reviewing session today:
- Add this codemod to the available migrations via @carbon/upgrade migrate, in packages/upgrade/src/upgrades.js
- Update the codemod to remove old flags that are no longer needed, like
enable-v11-release
. Mention this in the description of the migration in packages/upgrade/src/upgrades.js - Add some new test fixtures mixing old flags and new to validate that the right one are being removed
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 looks so good! 🔥 just three minor things I saw
packages/react/src/components/FeatureFlags/__tests__/FeatureFlags-test.js
Outdated
Show resolved
Hide resolved
152bfee
to
ab7bc72
Compare
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.
There's one more spot where enable-use-controlled-state-with-value
is used that could probably be deleted, it's not used anywhere else in the styles
'enable-use-controlled-state-with-value': true, |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17266 +/- ##
==========================================
- Coverage 76.95% 76.95% -0.01%
==========================================
Files 408 408
Lines 13979 13992 +13
Branches 4343 4300 -43
==========================================
+ Hits 10758 10768 +10
- Misses 3047 3051 +4
+ Partials 174 173 -1 ☔ View full report in Codecov by Sentry. |
23b8ff3
Closes #17243
New props to feature flag props were added here #17239
This introduces a new codemod to Convert FeatureFlags'
flags
object prop to individualboolean
propsand
deprecates
the existingflags
propChangelog
New
featureflag-deprecate-flags-prop
codemodtest fixtures
for all existing featureflags and tests to validateflags
prop for FeatureFlag is marked as deprecatedChanged
FeatureFlags/overview.stories.mdx
to document the codemodcodemod
to the flags deprecation warning messageTesting / Reviewing
jscodeshift -t featureflag-deprecate-flags-prop.js --parser=tsx _your_file_path_
curious to hear how you feel about this