Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: implement remote feature flag controller #12427
base: main
Are you sure you want to change the base?
feat: implement remote feature flag controller #12427
Changes from 25 commits
00a3bd1
b4f9cc8
cdd4a58
1787680
5299891
6204914
6f8d208
608f05b
a8ec092
01df041
c8c2af0
ca33006
27b32a3
d7aed0d
b80145d
9a67929
934a13a
faa6cb0
20c3b42
d22d1cb
ba1802e
5e97075
c838b21
426655e
ebc940e
8014b45
9a35568
8c305c1
c8495e5
d7d384c
b784e89
38292f7
7661b90
f5b754f
7bdcc81
e54d05d
b4fb866
f20e31a
6f709eb
82e2c88
54424a6
96ca13b
b5d9a6a
e12bfb6
50795f0
0126119
674962d
d451f5c
5cfbd03
9cd1bf0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Q: not sure if this change makes the mock in the test obsolete and how this test doesn't need any changes.
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.
Good catch, the tests for this hook need to be updated
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 surprised the test is passing given the changes in the hook... so maybe rewrite the test in a more robust way. A change like this should make it fail otherwise it probably means it's not testing anything but the mocks...
Check failure on line 1480 in app/core/Engine/Engine.ts
GitHub Actions / scripts (lint:tsc)
Check failure on line 1482 in app/core/Engine/Engine.ts
GitHub Actions / scripts (lint:tsc)
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 keep things consistent, let's follow the pattern that resembles that of what is currently in
Engine/controllers/accounts
The directory nameRemoteFeatureFlagController
is fine. We can keep the index file that you have and use it to re-export utils and anything else in the future.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.
In this case, it's debatable whether or not we should just put all of the functions in index or utils file. IMO, creating the utils file and re-exporting via index allows us to be more flexible in the future, where index can export other things such as constants, etc.
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.
addressed on f5b754f
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.
Nit: Maybe better to name this directory
platform
instead to preserve the team grouping, so that we can include other controllers here too in the futureThere 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 concerned about the potential tech debt we add when the team's name-based namings enter the codebase.
See another discussion around this topic.
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.
Well, we could think of it as "feature areas" instead perhaps, which roughly correlate to teams but are more long-lasting.
The purpose (as I understood it) of moving controller-related code to dedicated files was that it allowed us to assign more appropriate code owners (i.e. teams) and reduce code review burden and regression risk that comes from modifying core modules like
Engine.ts
.But the problem is that there are very few changes that affect just a controller. If we co-located multiple controllers owned by the same team, it would be more effective in minimizing cross-team impact/reviews for changes to how controllers interact with each other. If each controller is separate, then their interactions would be via
Engine.ts
still so we don't get that benefit.Single-controller directories don't seem helpful to me in reducing review burden or regression risk.
(Note: still not a blocker for me, just explaining my reasoning)
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 recognize the benefits of organizing the codebase in a feature-based structure. It helps with ownership and better outlines the relationship between components.
Namings that carry meaning for the codebase / product are long-lasting and do not need to be revisited if there's any maintainer's restructuring.
Responsibility areas can then be attributed to each team.
I'll leave it as is for now and follow up with an update when there's a decision on feature area names.
Check failure on line 5 in app/core/Engine/controllers/RemoteFeatureFlagController/index.ts
GitHub Actions / scripts (lint:tsc)
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.
add remote feature flag controller
init
unit testsThere 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.
is this a TODO?
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.
Thanks. Yes unit tests please!
Check failure on line 44 in app/core/Engine/controllers/RemoteFeatureFlagController/index.ts
GitHub Actions / scripts (lint:tsc)
Check failure on line 4 in app/core/Engine/controllers/RemoteFeatureFlagController/utils.ts
GitHub Actions / scripts (lint:tsc)
Check failure on line 159 in app/core/Engine/types.ts
GitHub Actions / scripts (lint:tsc)
This file was deleted.