-
Notifications
You must be signed in to change notification settings - Fork 2
Feature Flags Basic Implementation #289
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
base: master
Are you sure you want to change the base?
Conversation
yechs
left a comment
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.
TL;DR: Use Redux instead of React Context
Redux is complicated and there are many ways of writing them. We should talk tomorrow or some other time to introduce you to them
|
However, if you are curious about how Redux current works in our implementation. You might want to look at these files. At this point, I might as well write a short doc on it. HowTo: work with Modern Redux states in WSO-ReactKey ConceptsThe official Redux tutorial is also helpful. Read the following article to make sure you understand these terms: reducer, action, dispatch, selectors. Make sure to read modern Redux (using Redux Toolkit, or RTK)If you are searching for external documentation/tutorial. Make sure you are reading the modern Redux codes (with Redux Toolkit) instead of the old Redux (using React-Redux, which requires lots of boilerplates). Specifically, we are adopting the Slices style. Read more: Reducer and Action definition
Redux store configuration & PersistenceThe Redux store is configured in Lines 27 to 32 in 447ca37
In the file we also persist the reducer so its states are persisted through page refreshes. Lines 17 to 20 in 447ca37
The file creates two React Hooks ( useAppDispatch and useAppSelector) to be used in actual pagesLines 60 to 65 in 447ca37
Usage ExampleAn example usage is in wso-react/src/components/App.tsx Lines 52 to 56 in 447ca37
Write (by dispatching actions): wso-react/src/components/App.tsx Lines 75 to 82 in 447ca37
|
merging unconflicting changes from master
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.
TL;DR:
- Redux hooks and typing
- scope protect the admin page
src/lib/featureFlagSlice.ts
Outdated
| toggleFeatureFlag: ( | ||
| state, | ||
| action: PayloadAction<keyof FeatureFlagsState> | ||
| ) => { | ||
| const flagKey = action.payload; | ||
| if (flagKey in state) { | ||
| state[flagKey] = !state[flagKey]; | ||
| } | ||
| }, |
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.
I'm a little worried about toggling, because it is not idempotent (and also because redux is async).
Maybe the action should take in both flagKey and newState?
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.
I don't understand the idea here, the values can only be "on" or "off", why does toggling not make sense?
|
all changes requested changes are implemented besides changing the toggle, I don't understand exactly why that change would be necessary. A given feature should be on or off, and there is nothing to change to besides the other? |
True, but the toggle action is not idempotent. I did some more search and it looks like redux actions are not strictly required to be idempotent. Also imagine the case where the data is corrupted (you never know what the browser/user would do to your stored data...). |
|
I would also add that boolean states are generally bad practice. It's best to store Enum states for clarity and in case we would like to support other feature states. For example we might want to support a senior-only state or a student-only state. |
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.
To Change:
- Route name
- avoid magic keywords
Need Discussion:
- typing of states
- frontend/backend logic divide
src/components/App.tsx
Outdated
| <Route | ||
| path="admin" | ||
| element={ | ||
| <RequireScope token={apiToken} name="admin.dashboard"> |
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 route name should be just admin (since the path name is also just /admin)
| export enum FFState { | ||
| Enabled = "Enabled", | ||
| Disabled = "Disabled", | ||
| Pending = "Pending", | ||
| } | ||
|
|
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.
Regarding typing here, I think we need some more discussions.
@AndrewMuh, do you think it's a better idea if we give each flag its own type for granular state mangament?
If so, should we implement it in union types? (given that enum cannot be extended) Or totally separate types?
Also, do you think the rendering logic like senior-only should be handled at backend or front-end?
This question is important, because it will affect how FeatureFlagElement should be implemented (in Nav.tsx)
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.
Honestly thinking about it now, I think all flags should be either Enabled or Disabled (I still think enums, but only these two). Things like senior only or student only should be done through a "featureIsSeniorOnly" feature flag. This simplifies implementation in the frontend/db: just check ephmatchEnabled and ephmatchIsSeniorOnly for example.
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.
As for rendering logic, the frontend should restrict the rendering of a component using the FF, and the backend should reject invalid requests based on the FF.
AndrewMuh
left a comment
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.
fix small comments
|
I added some styling to dashboard.tsx -- I probably should have made a new pr but I didn't want to branch off of another pr |
yechs
left a comment
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.
So far so good.
Some more code will need to be added on backend integration
- Admin Dashboard: when flags are changed, send request to update them in backend API server
- All pages (perhaps in App.tsx, similar to
updateAPI()): on page load, send request to backend API for current feature flags
Related backend PR: https://github.com/WilliamsStudentsOnline/wso-go/pull/201
Basic implementation of feature flags and UI to use them, I implemented them for the about, wiki, and FAQ pages, there is also infrastructure to implement the feature flags for nav elements too. Big picture you add flags to components/FeatureFlagsConfig and then can edit them in the admin dashboard.
Sorry for the big PR, I got a bit carried away