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
Add sample feature flagging system to next.js template #259
Add sample feature flagging system to next.js template #259
Changes from all commits
f9767d6
353630f
6e20779
ed8be4d
4bbe4fc
3f03b05
682ba3f
4c00a33
5608096
4bbaf3d
8e4ea30
121a839
072e5c9
96a1067
22af91b
ee9ebc7
5960077
b85be42
211a22c
19bfc25
14e7f66
f3eb517
ebd85a6
5e7bbd3
d5bc09e
cc4f457
ea37495
4ef4d5f
baa0f9c
04958d5
e03f4bc
e8033ee
f8abfea
68da9d5
354b7ec
a3768f2
dcdf950
6b0d455
6e0fe93
90e4a95
e038ff4
62b1f5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 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 forgetServerSideProps
so the feature flag check is never executed in a test, but theNODE_ENV
check would be useful for when a project adds test coverage for code that callsisFeatureEnabled
.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 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.
would that address the issue you mentioned without a NODE_ENV check?
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.
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.