-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Actions: Add devalue
for serializing complex values
#11593
Conversation
🦋 Changeset detectedLatest commit: c4130c7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
callAction: APIContext['callAction']; | ||
actionResult?: ReturnType<APIContext['getActionResult']>; | ||
actionResult: SerializedActionResult; | ||
actionName: string; |
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.
Moved the getActionResult
constructor to render-context
. This ensures only serializable values are sent through locals
for edge middleware support. It also simplified our code in the process by removing that nextLocalsStub
utility!
Would URLs be a problem due to Rich-Harris/devalue#79? |
@Fryuni Yes it would be. I recall we special case this for content collections. I'll see if we can reuse that code. Update: added! Pretty easy to do |
message: body.message, | ||
code: ActionError.statusToCode(body.status), |
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.
Should we also anticipate if body
is not an object? Or if the property access returns undefined?
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.
Yeah that's a good call. We should add a check
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.
Update: I just realized this was the base case when an Action error code not be parsed. I decided to simply return INTERNAL_SERVER_ERROR
for that case, since that is unexpected.
6d0aff1
to
e707d5d
Compare
I wonder if we should abstract our special devalue usage so that it's consistent across the different places we use it. |
@ascorbic By "special devalue usage," do you mean for Actions specifically or Astro in general? I tried to consolidate everything into those serialize and deserialize functions for Actions. |
@bholmesdev I mean in general. We're now using it in actions, content collection schema and it will also be used in the content layer data store. Not a blocker, but it would be good to not re-implement it every time. |
@ascorbic Got it, agreed. I noticed that we're using eval / uneval for content collections and stringify / parse for actions, which is a difference. This is because content collections outputs a JS module (so eval is safe) while actions needs to serialize over the wire (so eval is unsafe). I'd prefer to keep using eval for content collections due to this note in the devalue readme:
Think it's best to keep separate for now. If you can abstract for content collections v2, please do! |
37b2823
to
c4130c7
Compare
Changes
Standardize our Actions serialization strategy. This introduces
devalue
to allow complex values like Dates and Sets, and adds serialization to middleware results for edge middleware support.serializeActionResult
anddeserializeActionResult
utilsTesting
Docs
N/A - should continue to work as expected. The RFC did not specify supported values