chore: localstorage pr feedback #327
Conversation
…replaced initializing event with identifying. removed stream and streamingDisabled.
yusinto
requested review from
kinyoklion and
louis-launchdarkly
as code owners
| client.on('ready', () => { | ||
| setState({ client }); | ||
| }); |
There was a problem hiding this comment.
ready has been deleted in favor of a simpler design relying only on change.
There was a problem hiding this comment.
This was used as the change callback argument but that's been simplified in favor of a string array of flag keys. This is no longer needed.
| // I'm not sure why but both runAllTimersAsync and runAllTicks are required | ||
| // here for the identify promise be resolved | ||
| await jest.runAllTimersAsync(); | ||
| jest.runAllTicks(); |
There was a problem hiding this comment.
This could be a bug in jest fake timers.
| } else { | ||
| // manually resolve identify | ||
| this.logger.debug('No changes from PUT'); | ||
| identifyResolve(); |
There was a problem hiding this comment.
Callout: manually resolve the identify promise if there are no changes.
| deserializeData: JSON.parse, | ||
| processJson: async (dataJson: Flags) => { | ||
| this.logger.debug(`Streamer PUT: ${Object.keys(dataJson)}`); | ||
| if (initializedFromStorage) { |
There was a problem hiding this comment.
As discussed we'll always run the synchronization logic and emit change when there's a change.
| this.logger.debug('Initializing all data from storage'); | ||
| this.logger.debug('Using storage'); | ||
|
|
||
| const changedKeys = calculateFlagChanges(this.flags, flagsStorage); |
There was a problem hiding this comment.
Again, like my comment above, always run the sync logic.
| * - `"ready"`: The client has finished starting up. This event will be sent regardless | ||
| * of whether it successfully connected to LaunchDarkly, or encountered an error | ||
| * and had to give up; to distinguish between these cases, see below. | ||
| * - `"initialized"`: The client successfully started up and has valid feature flag | ||
| * data. This will always be accompanied by `"ready"`. | ||
| * - `"failed"`: The client encountered an error that prevented it from connecting to | ||
| * LaunchDarkly, such as an invalid environment ID. All flag evaluations will | ||
| * therefore receive default values. This will always be accompanied by `"ready"`. |
There was a problem hiding this comment.
ready has been removed. initialized is replaced by identifying. failed has been removed, in favor of error.
There was a problem hiding this comment.
I moved these types to its own folder under types/index.ts so they are easier to reuse.
There was a problem hiding this comment.
I removed the config options stream and streamingDisabled for now. For this alpha, streaming will always be on.
| expect(json).toEqual(mockResponse); | ||
| }); | ||
|
|
||
| test('report', async () => { |
There was a problem hiding this comment.
I removed useReport from this alpha for now.
yusinto
merged commit 713221e
into
yus/sc-225809/use-localstorage-for-bootstrapping-rn-sdk
This implements feedback from #326.
changeevents.readyevent and usechangeinstead.changeis now just a string array of flag keys.streamanduseReportfor now for this alpha.