-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor: to use combine adapters with one #1971
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/ckfmqm8mn |
🦋 Changeset detectedLatest commit: 9b37abb The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
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.
Looks very promising. Thanks for pushing this 🤗
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
I'm trying to review the implementation but I don't understand the application of |
The idea is to homogenise the internal use of feature flags. We also have a fe-chapter issue about the topic. The status quo internally is that we have:
Both are rightfully configured in different systems. 1. is in our k8s files as a ConfigMap. 2. is in LaunchDarkly. Now for either system or type of flag we use a different "backing system". For 1. we use |
aaah clear. |
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.
Looks good overall. just a question
// This value is hard-coded here because we want to make sure that the | ||
// app uses our account of LD. The value is meant to be public, so there | ||
// is no need to be concerned about security. | ||
const ldClientSideIdProduction = '5979d95f6040390cd07b5e01'; | ||
|
||
combineAdapters.combine([ldAdapter, graphqlAdapter]); | ||
|
||
const getRequestHeaders = (props: Props) => |
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.
apologies if I confuse things.
- the
getRequestHeaders
is supposedly called withadapterArgs
. - Here, we accept
props
..
Where did props come from?
maybe this is related to why you had to // @ts-expect-error
?
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. That's a good catch!
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.
ping me when you resolve it ;)
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.
Sure. I am doing this on the side. So might take some time :)
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
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.
Nice, I like it. It's also much simpler now!
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/setup-flop-flip-provider/setup-flop-flip-provider.tsx
Outdated
Show resolved
Hide resolved
8b741ab
to
f8f7db5
Compare
f8f7db5
to
56d45d4
Compare
56d45d4
to
e56fea6
Compare
ea7bb41
to
251790b
Compare
I think I am done on the |
251790b
to
8fef400
Compare
8fef400
to
88e07f4
Compare
@@ -81,15 +81,10 @@ const parseFlags = (fetchedFlags: TFetchedFlags): TParsedHttpAdapterFlags => | |||
]) | |||
); | |||
|
|||
type TAdditionalEnvironmentProperties = { |
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 felt that anything that app-kit supports should be additional conceptually...
@@ -101,11 +96,11 @@ export const SetupFlopFlipProvider = (props: Props) => { | |||
[allMenuFeatureToggles.allFeatureToggles, props.flags] | |||
); | |||
React.useMemo(() => { |
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.
Simple code is good code I think.
cbe4626
to
ea5d9d4
Compare
@@ -143,7 +143,7 @@ export const SetupFlopFlipProvider = (props: Props) => { | |||
}); | |||
|
|||
return parseFlags(response.data); | |||
} | |||
}, |
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.
😄
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, looks good to me now. Don't forget to add the changeset.
Yup. I want to beta test this a bit more. So I'll not merge it for now. I need time for that. |
dd941f6
to
60b0d12
Compare
Ok. I did extensive testing internally. This works very nicely. Anything left here @emmenko otherwise I can start integrating. I will share details of my integrating testing internally. |
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.
Looks good! Looking forward to see this in action 🚀
Co-authored-by: Nicola Molinari <nicola.molinari@commercetools.de>
Summary
This pull request experiments with the
graphql-adapter
andcombine-adapters
from@floplfip
.We don't have to use this functionality but I would like to propose it through this internally.