Expose LDProvider as a standalone component #28
Conversation
There was a problem hiding this comment.
@nimi thank you for submitting this PR! Exposing the Provider component definitely makes sense and it is something that we agree should be included in the react sdk.
I have left some feedback here which are hopefully useful. In addition to the in-line comments, is it also possible to add an example app to the examples folder please? There are currently an example app each for asyncWithLDProvider and withLDProvider. It will be tremendously helpful to other users to have another example app demonstrating this new Provider component.
Also for completeness, is it also possible to add documentation for this new Provider component to the react sdk docs please? You can submit a PR via the Edit on github button on that page.
Thank you again.
| @@ -1,16 +1,12 @@ | |||
| import * as React from 'react'; | |||
| import camelCase from 'lodash.camelcase'; | |||
| import { LDClient, LDFlagSet, LDFlagChangeset } from 'launchdarkly-js-client-sdk'; | |||
| import { defaultReactOptions, ProviderConfig, EnhancedComponent } from './types'; | |||
There was a problem hiding this comment.
I think it's safe to remove EnhancedComponent now that it is no longer used here.
| if (options) { | ||
| const { bootstrap } = options; | ||
| if (bootstrap && bootstrap !== 'localStorage') { | ||
| const { useCamelCaseFlagKeys = defaultReactOptions.useCamelCaseFlagKeys } = reactOptions || {}; |
There was a problem hiding this comment.
It seems the logic to generate react options based on the default and user values is being used multiple times in this component. Perhaps we can optimize this to a single function which can be re-used, thus removing duplication:
// new function to generate react options which can be re-used
getReactOptions = () => ({ ...defaultReactOptions, ...this.props.reactOptions });
// then on lines 42, 58 and 68, the usage becomes
const { useCamelCaseFlagKeys } = this.getReactOptions();You can also safely remove the destructured prop reactOptions on line 32 above if you do this since it is no longer used.
|
Hi @yusinto! Thanks so much for the review and follow up! My apologies for the delay in my response I can definitely take care of the feedback mentioned with both docs and code cleanup. I'll address those concerns and request review when I've updated. Cheers, |
|
Hi guys, Thanks guys, |
|
We merged #31 just now -- I'm closing this PR in favor of that one. |
update JS SDK to 2.18.1
Requirements
Related issues
#27
Describe the solution you've provided
I've lifted the LDProvider class component used inside the
withLDProviderHOC into a standalone class component export. I've done my best to ensure that that there no new behavior changes were made and only on the task of moving the Provider code into a place where it can be exported in the public API of this SDK.I've also moved around the unit tests and shifted coverage to the standalone
LDProvider.Describe alternatives you've considered