Issue 0 implement feature flagging #43
Conversation
…implement-feature-flagging # Conflicts: # package-lock.json # package.json
|
|
||
| const file = './codeclimbers.identity' | ||
|
|
||
| export class FileStore implements Store { |
There was a problem hiding this comment.
Nice implementation! Looks like this should work well. curious why you chose to write this to file instead of the db? It does feel weird to create an entire table to store and retrieve one record
There was a problem hiding this comment.
The main reason was preventing the server and config locking each other out of the database by either both accessing the file or by them cross-depending on each other to start up.
| @@ -0,0 +1,30 @@ | |||
| export class Flag { | |||
There was a problem hiding this comment.
Seems like a good candidate for unit tests
There was a problem hiding this comment.
The flag itself will be quick to test, would do this in the next step(including testing more stuff)
src/config/src/flagsmith.ts
Outdated
| data[0].json(), | ||
| data[1].json(), | ||
| ]) | ||
| console.log(envList, idList) |
src/config/src/flagsmith.ts
Outdated
| } catch (error) { | ||
| // @todo logging | ||
| } | ||
| console.log(this.flags) |
| // @todo log somewhere | ||
| return new Flag(flagName, false, false, false, false) | ||
| } | ||
| private async loadFlags() { |
There was a problem hiding this comment.
Decent amount going on here. Are there any behaviors here we can test?
There was a problem hiding this comment.
Looks like there are network calls here that might make testing more difficult and I don't really care to test networking rn. Potentially could test the second half if that half is cohesive as its own function
There was a problem hiding this comment.
I'm happy to refactor that, would just due to size reasons not want to do the testing and refactoring here.
| "url": "https://github.com/idrinth" | ||
| }, | ||
| "scripts": { | ||
| "start": "node index.js", |
There was a problem hiding this comment.
What will be managing this websocket server when the CLI is distributed? Will oclif start and manage both as a systemd?
There was a problem hiding this comment.
that would be the intention, yes
The Pull Request is ready
issue-123-enable-x-does-not-disable-yIntention
With this change I intend to provide a basic implementation for a feature flagging service.
Review Points
Please take extra care reviewing the error handling, since I had to provide a lot of fallbacks for broken configurations or unreachable remote hosts.
The code follows best practices
anytype usedNotes
This current solution uses websockets to prevent the clients from having to pull. We can easily move it to a rest-api if desired.