-
Notifications
You must be signed in to change notification settings - Fork 293
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
Adding PipelineTransform class #2553
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
7f09712
to
52722e5
Compare
src/cloudflare/internal/test/pipeline-transform/transform-test.js
Outdated
Show resolved
Hide resolved
3388c5f
to
60da101
Compare
60da101
to
a8a094c
Compare
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.
Mostly looks good! Just a few small things.
obj.dispatcher = 'was here!'; | ||
} | ||
|
||
return batch; |
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.
Minor, but it'd be nice to test the async nature of this as well -- e.g. modifying each obj a little more after waiting on a scheduler.yield()
or something like that.
} | ||
|
||
#sendJson(data: object[]): JsonStream { | ||
if (!(data instanceof Array)) { |
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'd imagine an argument could be made for optionally supporting null and/or undefined instead of an empty array, but that's entirely a product question for you / the team, and it is indeed easier to start strict and loosen this later than the opposite.
const readable = new ReadableStream<Uint8Array>({ | ||
start(controller): void { | ||
for (const obj of data) { | ||
const encoded = encoder.encode(`${JSON.stringify(obj)}\n`); |
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.
In terms of validating the response from the customer's code, it's also possible for them to return values that will throw when passed to JSON.stringify
, and we'd probably benefit from handling them more gracefully than just an uncaught exception?
Although then again in the instanceof Array
check above all we do is throw an exception, so actually maybe this is already fine and we just need the calling code to detect this case correctly?
This PR adds a class called PipelineTransform which allows us to test PipelineTransform implementations before sending them real data from "Cloudflare Pipelines".
It also means that you don't have to deal with streams directly and can just work with JSON/Javascript Objects.