Add sample feature flagging system to next.js template #259

aligg · 2023-12-04T17:06:45Z

Changes

Add a service class for retrieving flags from evidently
Add example consuming the flags
Updated docs
tests

Context for reviewers

This is followup / related work alongside last week's platform infra changes.

Testing

I'd love some help testing this out against the platform infra.

lorenyu

Nice, looks great! Left a high level comment on the architectural pattern

app/src/services/FeatureFlagManager.ts

app/__mocks__/@aws-sdk/client-evidently.ts

app/src/services/FeatureFlagManager.ts

app/.env.development

app/src/pages/index.tsx

app/src/services/FeatureFlagManager.ts

app/__mocks__/@aws-sdk/client-evidently.ts

app/.env.development

app/src/services/feature-flags/setup.ts

… into aligg/ff # Conflicts: # app/src/pages/index.tsx

docs/feature-flagging.md

app/src/i18n/messages/en-US/index.ts

app/src/pages/index.tsx

app/src/services/feature-flags/FeatureFlagManager.ts

app/src/services/feature-flags/LocalFeatureFlagManager.ts

package-lock.json

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

:

sawyerh

This is looking good, thanks for iterating and debugging things! I was able to test this successfully locally using the Platform IAM credentials (left a comment with relevant instructions).

docs/feature-flags.md

sawyerh · 2023-12-08T01:47:21Z

app/tests/pages/index.test.tsx

@@ -2,11 +2,13 @@ import { axe } from "jest-axe";
 import Index from "src/pages/index";
 import { render, screen } from "tests/react-utils";

+jest.mock("src/services/feature-flags/setup");


I think we can get rid of this line as well as the __mocks__ file now since the moduleNameMapper change fixed the original issue.

sawyerh · 2023-12-08T01:49:19Z

app/src/services/feature-flags/setup.ts

+  isFeatureEnabled(feature: string, userId?: string): Promise<boolean>;
+}
+
+export const manager: FlagManager = process.env.FEATURE_FLAGS_PROJECT


I think we may still want to add the NODE_ENV !== "test" check here, since we are getting rid of the Jest mock. It's not causing issues at the moment since there's no test for getServerSideProps so the feature flag check is never executed in a test, but the NODE_ENV check would be useful for when a project adds test coverage for code that calls isFeatureEnabled.

I prefer not to do environment checks in our source code since that breaks 12 factor app principles. A couple of reasons: (1) if the logic in the code has branches based on environment variables, it means that we have less confidence that the code branch that's covered in one environment (e.g. automated test suite) would work the same as the code branch that's covered when deployed to production and. Relatedly, in terms of test coverage tools, one entire branch of code will not be covered. (2) it makes it harder to implement automated integration tests if the test suite is forced to not use the AWS flag manager.

If we want to be more explicit about when we're using one flag manager type over the other without having to comment out or remove the FEATURE_FLAGS_PROJECT env var, we could always add an extra env var like FEATURE_FLAG_MANAGER_TYPE that is either "aws" or "local" e.g.

manager = process.env.FEATURE_FLAG_MANAGER_TYPE === "aws" ? new AWSFeatureFlagManager(process.env.FEATURE_FLAGS_PROJECT) : new LocalFeatureFlagManager();

would that address the issue you mentioned without a NODE_ENV check?

Cross-posting from Slack:

Sorry, I caused confusion with an inaccurate statement — Next.js only loads env vars into the test environment from the .env file, but it doesn’t load the vars in files like .env.development or .env.local so I think ultimately Ali your existing condition that just checks the FF project var would be enough.

app/tests/pages/index.test.tsx

lorenyu · 2023-12-06T01:42:01Z

docs/feature-flagging.md

+# Feature flagging
+
+- [AWS Evidently](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-Evidently.html) is used for feature flagging
+- For more information about the decision-making behind using Evidently, [this infra ADR is available](https://github.com/navapbc/template-infra/blob/main/docs/decisions/infra/0010-feature-flags-system-design.md)


The PR is merged, so you can probably reference the ADR directly on the main branch

lorenyu · 2023-12-06T01:45:57Z

docs/feature-flagging.md

+## Local development
+
+Out-of-the-box, local calls to Evidently will fail with feature flag values defaulting to false. If you want to test Evidently locally, use your AWS IAM credentials. Once you set AWS environment variables locally for the environment you wish to connect to, calls to Evidently will succeed


i think we should provide a local feature flag implementation that doesn't rely on evidently. config files and environment variables are the approaches that jump to mind, but if there's an approach that leverages Next.js's local hot reloading that'd be nice to have.

app/__mocks__/@aws-sdk/client-evidently.ts

app/src/services/feature-flags/setup.ts

lorenyu · 2023-12-08T15:47:45Z

app/src/services/feature-flags/setup.ts

+  isFeatureEnabled(feature: string, userId?: string): Promise<boolean>;
+}
+
+export const manager: FlagManager = process.env.FEATURE_FLAGS_PROJECT


I prefer not to do environment checks in our source code since that breaks 12 factor app principles. A couple of reasons: (1) if the logic in the code has branches based on environment variables, it means that we have less confidence that the code branch that's covered in one environment (e.g. automated test suite) would work the same as the code branch that's covered when deployed to production and. Relatedly, in terms of test coverage tools, one entire branch of code will not be covered. (2) it makes it harder to implement automated integration tests if the test suite is forced to not use the AWS flag manager.

If we want to be more explicit about when we're using one flag manager type over the other without having to comment out or remove the FEATURE_FLAGS_PROJECT env var, we could always add an extra env var like FEATURE_FLAG_MANAGER_TYPE that is either "aws" or "local" e.g.

manager = process.env.FEATURE_FLAG_MANAGER_TYPE === "aws" ? new AWSFeatureFlagManager(process.env.FEATURE_FLAGS_PROJECT) : new LocalFeatureFlagManager();

would that address the issue you mentioned without a NODE_ENV check?

lorenyu · 2023-12-08T15:48:54Z

I meant to just add one inline comment in response to a thread, not sure why it posted an entire review with what seems to be outdated comments. Sorry!

edit: oh it looks like i had review comments pending that i just never remembered to submit. maybe worth looking at the comments but apologies in advance if any of them are no longer relevant.

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

aligg requested review from sawyerh, lorenyu, rocketnova and chrisvincentnava December 4, 2023 17:06

lorenyu reviewed Dec 4, 2023

View reviewed changes

sawyerh reviewed Dec 5, 2023

View reviewed changes

aligg added 21 commits December 6, 2023 15:03

add FF manager

f9767d6

format

353630f

tweaks

6e20779

update logging

ed8be4d

format

4bbe4fc

tweak config

3f03b05

refactor a few things

682ba3f

tweaking

4c00a33

foo

5608096

typing errs

4bbaf3d

add mocks

8e4ea30

format

121a839

variable names, console err, addl pr feedback

072e5c9

remove region

96a1067

format

22af91b

rename class

ee9ebc7

a few more pr review comments

5960077

add docs

b85be42

add back evidently mock for now

211a22c

add back mock w/ format

19bfc25

rebase

14e7f66

aligg force-pushed the aligg/ff branch from 9e91405 to 14e7f66 Compare December 6, 2023 23:27

aligg added 2 commits December 6, 2023 15:27

format

f3eb517

add additional structure for feature flagging

ebd85a6

aligg added 5 commits December 6, 2023 15:47

linting

5e7bbd3

spurious patch file

d5bc09e

update docs

cc4f457

spurious patch file again

ea37495

tests

4ef4d5f

aligg requested a review from lorenyu December 7, 2023 00:18

sawyerh reviewed Dec 7, 2023

View reviewed changes

app/src/services/feature-flags/setup.ts Outdated Show resolved Hide resolved

sawyerh and others added 2 commits December 7, 2023 11:16

Merge branch 'main' of github.com:navapbc/template-application-nextjs…

baa0f9c

… into aligg/ff # Conflicts: # app/src/pages/index.tsx

rename

04958d5

sawyerh reviewed Dec 7, 2023

View reviewed changes

aligg and others added 9 commits December 7, 2023 14:45

Update docs/feature-flagging.md

e03f4bc

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

Update docs/feature-flagging.md

e8033ee

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

Update app/src/pages/index.tsx

f8abfea

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

Update app/src/pages/index.tsx

68da9d5

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

Update app/src/services/feature-flags/LocalFeatureFlagManager.ts

354b7ec

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

rename feature-flags docs and camel case keys

a3768f2

make userId optional and remove extra lockfile

dcdf950

update docs, jest config, and typo on project env var

6b0d455

format

6e0fe93

:

sawyerh approved these changes Dec 8, 2023

View reviewed changes

lorenyu reviewed Dec 8, 2023

View reviewed changes

Update docs/feature-flags.md

90e4a95

Co-authored-by: Sawyer Hollenshead <git@sawyerh.com>

aligg mentioned this pull request Dec 8, 2023

Feature flagging: Add local feature flagging implementation that doesn't rely on evidently #263

Closed

aligg added 2 commits December 8, 2023 09:56

update env.development file, remove line from test and update readme

e038ff4

add a test for get server side props

62b1f5b

aligg merged commit 7c7d5f5 into main Dec 8, 2023
6 checks passed

aligg deleted the aligg/ff branch December 8, 2023 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sample feature flagging system to next.js template #259

Add sample feature flagging system to next.js template #259

aligg commented Dec 4, 2023 •

edited

Loading

lorenyu left a comment

sawyerh left a comment

sawyerh Dec 8, 2023

sawyerh Dec 8, 2023

lorenyu Dec 8, 2023

sawyerh Dec 8, 2023 •

edited

Loading

lorenyu Dec 6, 2023

lorenyu Dec 6, 2023

aligg Dec 8, 2023

lorenyu Dec 8, 2023

lorenyu commented Dec 8, 2023 •

edited

Loading

		## Local development

		Out-of-the-box, local calls to Evidently will fail with feature flag values defaulting to false. If you want to test Evidently locally, use your AWS IAM credentials. Once you set AWS environment variables locally for the environment you wish to connect to, calls to Evidently will succeed

Add sample feature flagging system to next.js template #259

Add sample feature flagging system to next.js template #259

Conversation

aligg commented Dec 4, 2023 • edited Loading

Changes

Context for reviewers

Testing

lorenyu left a comment

Choose a reason for hiding this comment

sawyerh left a comment

Choose a reason for hiding this comment

sawyerh Dec 8, 2023

Choose a reason for hiding this comment

sawyerh Dec 8, 2023

Choose a reason for hiding this comment

lorenyu Dec 8, 2023

Choose a reason for hiding this comment

sawyerh Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

lorenyu Dec 6, 2023

Choose a reason for hiding this comment

lorenyu Dec 6, 2023

Choose a reason for hiding this comment

aligg Dec 8, 2023

Choose a reason for hiding this comment

lorenyu Dec 8, 2023

Choose a reason for hiding this comment

lorenyu commented Dec 8, 2023 • edited Loading

aligg commented Dec 4, 2023 •

edited

Loading

sawyerh Dec 8, 2023 •

edited

Loading

lorenyu commented Dec 8, 2023 •

edited

Loading