-
Notifications
You must be signed in to change notification settings - Fork 9
[NET-182] Add detectFields implementation to Stream #196
Conversation
NET-182 Replace API call with field detection implementation in JS client
In the JS client, there's a method that calls an API endpoint The JS client should instead implement the field detection on client level:
The inspection logic used to be in the broker and was something like this: |
|
The logic is so simple that it's now part of existing |
…tream info from server.
39d6fc1 to
2ce2157
Compare
|
Rebased, added types. LGTM. |
| getEndpointUrl(this._client.options.restUrl, 'streams', this.id, 'detectFields'), | ||
| this._client.session, | ||
| ) | ||
| // Get last message of the stream to be used for field detecting |
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 wonder if we should try to be a bit more fault tolerant, or smarter, about handling cases where the data is shaped differently e.g. grab the last n messages and base the type on a combination of the n, or raise a warning if they aren't all of the same time.
None of this is enforced really though so maybe it doesn't matter.
src/stream/index.js
Outdated
| }) | ||
|
|
||
| // Save field config back to the stream | ||
| this.config.fields = fields |
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.
Hm, what happens if detectFields is called twice in parallel and the async calls resolve out of order? Going forward perhaps could ensure only a single call can be outstanding
Field detection support was removed from backend a while ago so
Stream.detectFields()call is not working. This PR adds this simple logic usingFieldDetectorinto the client itself.