Skip to content
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.

Sign up for GitHub

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

Jump to bottom

date handling #1504

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

date handling #1504

wants to merge 3 commits into from
+1,506 −627

Conversation

eandre
Copy link
Member

@eandre eandre commented Oct 18, 2024

  • runtimes/{core,js}: refactor schema encode/decode
  • Add date support

@eandre
runtimes/{core,js}: refactor schema encode/decode
8192663 
Introduce a pair of types, `PValue` and `PValues`. The types
allow us to introduce dedicated semantic types that go beyond
plain JSON types.

As a result, since the wire protocol is still JSON, decoding JSON
into these types requires an API schema (so that we know e.g.
whether a string is "just a string" or should be a datetime.

This commit introduces the concept and refactors the runtimes
to use it. The next commit adds support for dates.
@encore-cla
Copy link

encore-cla bot commented Oct 18, 2024

All committers have signed the CLA.

fredr
fredr approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@fredr fredr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

runtimes/core/src/pubsub/mod.rs
@@ -24,7 +24,7 @@ pub type MessageId = String;

pub struct MessageData {
pub attrs: HashMap<String, String>,
pub body: Option<serde_json::Value>,
// pub body: Option<serde_json::Value>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete this? or is it going to be re-added at some point?

fredr
fredr reviewed Oct 22, 2024
View reviewed changes
runtimes/js/src/pvalue.rs
}

impl FromNapiValue for PVals {
unsafe fn from_napi_value(env: sys::napi_env, napi_val: sys::napi_value) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code panics if the value is undefined. I had similar problems when parsing the error details, and had to parse it to JsUnknown first and then match on the type to check that it was an JsObject before coercing into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredr fredr fredr approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eandre @fredr