-
Notifications
You must be signed in to change notification settings - Fork 172
chore: Support feature flags in SSR #2028
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
Conversation
const { getGlobalFlag } = globalFlags; | ||
|
||
const awsuiGlobalFlagsSymbol = Symbol.for('awsui-global-flags'); |
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.
Some unique symbol
type madness here. Symbol.for(key)
returns different symbols even if the key is the same: microsoft/TypeScript#35909
So I needed to add an import of awsuiGlobalFlagsSymbol
. I also could not get it from const { ... } = globalFlags
because it also would not be the same unique symbol
.
But on the upside I managed to remove copies of these interfaces below while keeping everything type safe
src/internal/utils/global-flags.ts
Outdated
return typeof window !== 'undefined' ? window : globalThis; | ||
} | ||
|
||
function readFlag(window: any, flagName: keyof GlobalFlags) { |
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.
Decided to go with any
here, because as ExtendedWindow
all around this file were not adding much type safety anyway. But at least the code is more compact now
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.
We could use unknown
for the parameter type and cast it to FlagsHolder
inside the function. That way, we'll get type checks on the window?.[...
line
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.
Could you show an an example of the readFlag(window: unknown)
implementation you ideally want?
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.
Found a version from you: 7048f87#diff-5b9d78b3482fe99531e24bedf68759f075aeca010bbcae8797110683d36eb35cR17-R18
I get it now, updated the implementation
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.
For example:
function readFlag(object: unknown, flagName: keyof GlobalFlags) {
if (!object) return undefined;
const flagsHolder = object as FlagsHolder;
return flagsHolder[awsuiGlobalFlagsSymbol]?.[flagName];
}
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.
Makes sense, window: any
made the return type of readFlag
to be any
which is unexpected. Applied your suggestion.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
=======================================
Coverage 94.83% 94.84%
=======================================
Files 669 669
Lines 18113 18114 +1
Branches 6002 6003 +1
=======================================
+ Hits 17178 17180 +2
+ Misses 870 869 -1
Partials 65 65 ☔ View full report in Codecov by Sentry. |
70a7150
to
8394e86
Compare
src/internal/utils/global-flags.ts
Outdated
return typeof window !== 'undefined' ? window : globalThis; | ||
} | ||
|
||
function readFlag(window: any, flagName: keyof GlobalFlags) { |
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.
We could use unknown
for the parameter type and cast it to FlagsHolder
inside the function. That way, we'll get type checks on the window?.[...
line
expect(typeof window === 'undefined').toBe(true); | ||
}); | ||
|
||
test('returns undefined if there global flags are not defined', () => { |
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.
typo: "there" should probably be "the"
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.
All these names are copy from the original tests. Updated them in both places
8394e86
to
0437439
Compare
0437439
to
119bb70
Compare
Description
Make it possible to define feature flags in SSR
Related links, issue #, if available: n/a
How has this been tested?
Added an SSR test suite
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.