-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(useEventSource): add transformer #4953
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
base: main
Are you sure you want to change the base?
Conversation
|
||
/** | ||
* Data serialization options | ||
*/ | ||
serialization?: { | ||
/** | ||
* Whether to automatically parse JSON data | ||
* @default false | ||
*/ | ||
parseJSON?: boolean | ||
|
||
/** | ||
* Custom data transformer function | ||
* @param data Raw data from EventSource | ||
* @returns Transformed data | ||
*/ | ||
transform?: (data: any) => any | ||
} |
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.
IMO we should only have the transform/serializer.
we should add an optional generic to the options and use this for the serializer
serializer?: (v: unknown) => T
the serializer should default to:
serializer: (v) => v
The reason I don't like the parseJSON option is that it doesn't save much.
serialization?: {
parseJSON: true
}
vs
serializer: (v) => JSON.parse(v)
It is not clear to the user what should happen if we pass both.
serialization?: {
parseJSON: true,
transform: (v)=> /* ... */
}
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.
thank you for the feedback!
I modified the code according to your suggestion
how is it this time?
Thank you for your PR. |
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 so far. Some small adjustments.
Maybe we should rename it to: serializer
like useBase64
Thank you! |
There are no new commits. Maybe you forgot to push 😅 |
I forgot last time. Could you please do the code review again? |
@@ -183,14 +186,14 @@ export function useEventSource<Events extends string[], Data = any>( | |||
|
|||
es.onmessage = (e: MessageEvent) => { | |||
event.value = null | |||
data.value = e.data | |||
data.value = serialization(e.data) || null |
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.
data.value = serialization(e.data) || null | |
data.value = serialization(e.data) ?? null |
data.value = serialization(e.data) || null | ||
lastEventId.value = e.lastEventId || null |
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.
data.value = serialization(e.data) || null | |
lastEventId.value = e.lastEventId || null | |
data.value = serialization(e.data) ?? null | |
lastEventId.value = e.lastEventId ?? null |
const event: ShallowRef<string | null> = shallowRef(null) | ||
const data: ShallowRef<Data | null> = shallowRef(null) | ||
const data: ShallowRef<Data | TransformedData | null> = shallowRef(null) |
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.
const data: ShallowRef<Data | TransformedData | null> = shallowRef(null) | |
const data: ShallowRef<TransformedData | null> = shallowRef(null) |
how about now? |
/** | ||
* Reference to the latest data received via the EventSource, | ||
* can be watched to respond to incoming messages | ||
*/ | ||
data: ShallowRef<Data | null> | ||
data: ShallowRef<Data | TransformedData | null> |
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.
data: ShallowRef<Data | TransformedData | null> | |
data: ShallowRef<TransformedData | null> |
sorry, overlooked this one 🫠
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.
lgtm!
Thank you!
'https://event-source-url', | ||
[], | ||
{ | ||
serialization: (rawData) => JSON.parse(rawData), |
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.
We held an internal discussion. We want to use the serialiser to ensure consistency with the other composables.
Could you please adjust the PR?
Before submitting the PR, please make sure you do the following
fixes #123
).Description
Additional context
This enhancement addresses a common pain point when working with EventSource APIs that send JSON data as strings, requiring manual parsing in every usage. The implementation follows VueUse's existing patterns and maintains full backward compatibility.