[WIP] feat(core): PoC implementation of Remote Configuration #12090
Conversation
billyvg
force-pushed
the
feat-remote-config-init
branch
from
7c41bd8
to
590818a
Compare
This reverts commit a20a3c5.
| @@ -51,6 +51,7 @@ export function makeFetchTransport( | |||
| pendingBodySize -= requestSize; | |||
| pendingCount--; | |||
| return { | |||
| response, | |||
There was a problem hiding this comment.
TBD if we re-use the transport, but we'll need access to the response body (as well as changing http req method to GET)
packages/core/src/baseclient.ts
Outdated
| // sampleRate is used. then set client as ready after remote config is loaded and flush queue | ||
| // TODO: create/pass transport | ||
|
|
||
| const storage = this._createRemoteStorage(); |
There was a problem hiding this comment.
implemented by client sdks for local storage/caching
packages/core/src/baseclient.ts
Outdated
| // if (this.config.getSource() === 'DEFAULT' && blockForRemoteConfig && blockForRemoteConfig.timeout > 0) { | ||
| // new Promise(resolve => { | ||
| // setTimeout(resolve, blockForRemoteConfig.timeout); | ||
| // remoteConfigFetchPromise.then(resolve); | ||
| // }) | ||
| // .then(() => this.finishInit()) | ||
| // .catch(() => { | ||
| // // TODO: Error handling | ||
| // }); | ||
| // | ||
| // return; | ||
| // } |
There was a problem hiding this comment.
Can't do this 😬 , we'll have to apply new configs and have sdks restart if possible.
packages/core/src/baseclient.ts
Outdated
| // replaysSessionSampleRate: options.replaysSessionSampleRate, | ||
| // replaysOnErrorSampleRate: options.replatsOnErrorSampleRate, |
There was a problem hiding this comment.
not typed in browser sdk, will need to have the replay integration handle this.
size-limit report 📦
|
billyvg
changed the title
packages/core/src/remoteconfig.ts
Outdated
| // Use cached configuration if it exists | ||
| const cachedValue = storage.get(defaultConfigName); | ||
| if (cachedValue) { | ||
| _activeConfig = JSON.parse(cachedValue); |
There was a problem hiding this comment.
I think we should be putting JSON.parse/serialize into the storage object itself, and it's going to need try/catch guards around it
packages/core/src/remoteconfig.ts
Outdated
| let _lastUpdate: Date | undefined; | ||
| let _source: RemoteConfigSource = 'DEFAULT'; | ||
| let _hasUpdate: boolean = false; | ||
| let _state: RemoteConfigState = 'INITIALIZING'; |
There was a problem hiding this comment.
I can't find the dfn of RemoteConfigState so far.
I think this state machine in here is going to be really important to get right because any of the state names we export will be hard to change later, and testing the different transitions will give the confidence that things are working well
There was a problem hiding this comment.
I don't think that we need to expose these states, they'll be used internally
packages/core/src/remoteconfig.ts
Outdated
| } | ||
|
|
||
| /** @inheritdoc */ | ||
| function get<T>(defaultConfig: T): T { |
There was a problem hiding this comment.
It's a big cumbersome to pass in defaultConfig each time, unless there's something else for the public api.
Users could endup having to do:
const defaultConfig = [{key: 'is_enabled', value: false}];
const is_enabled = remoteConfigInst
.get(defaultConfig)
.find(feature => feature.key == 'is_enabled')
.value;
There was a problem hiding this comment.
Ah yeah this hasn't been updated for the new API, will be: get(key, defaultValue)
Do not merge
Initial proof of concept of how remote configuration could work in the JS SDKs.