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.

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

Generate an in-transit tree #39

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Generate an in-transit tree #39

merged 3 commits into from
Sep 27, 2023

Conversation

chee
Copy link
Member

@chee chee commented Sep 25, 2023

all works well. one remaining question:

do we need a runtime way of distinguishing one tree from another?

does the body node need to have a transit?: boolean field?

i don't think it does... i don't think we need that.

if we generate the [insert whatever we use to validate on c&m] types from the ContentTree.transit namespace then we're gucci, right?

@chee chee requested review from a team as code owners September 25, 2023 07:22
@chee chee requested review from serena97 and cebirmingham and removed request for a team September 25, 2023 07:22
README.md Outdated Show resolved Hide resolved
Comment on lines -269 to +320
picture?: ImageSetPicture
external picture: ImageSetPicture
Copy link
Member Author

Choose a reason for hiding this comment

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

here we mark the picture as external

Comment on lines +18 to +21
// in the full tree, externals are mandatory
const full = code.replace(/^(\s+)external (.+)$/gm, "$1$2")
// in the transit tree, externals must not be present
const transit = code.replace(/^\s+external (.+:).+$/gm, "")
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the entire logic for how the two namespaces are generated. remove the external modifier for the full tree, remove the entire property for the transit tree

Comment on lines +26 to +29
The `external` property modifier indicates that the specified field is absent
when the `content-tree` is in
[**transit**](#what-does-it-mean-to-be-in-transit), and required when the
**content-tree** is at rest.
Copy link
Member Author

Choose a reason for hiding this comment

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

this describes the new external property modifer. which works like the readonly property modifier in typescript, except it only exists in this readme

@chee chee requested a review from adgad September 25, 2023 07:46
@chee
Copy link
Member Author

chee commented Sep 25, 2023

message from chee on march 14th 2023: i think we really do need to do the Transit/Full content-tree split. i’m going to do it tomorrow

lol. today is March 15th.

Copy link
Collaborator

@adgad adgad left a comment

Choose a reason for hiding this comment

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

Neat!

I don't think we need any form of runtime validation in the spec IMO. If consumers wish to, they can use something like zod to validate against the generated types. Customer Products currently does that for the entire C&M response, so it should work for free once they designate bodyTree of having type ContentTree.transit.body.

Probably want a 👍 from one of the other teams here as well

@chee chee merged commit 7910a4d into main Sep 27, 2023
2 checks passed
@chee chee deleted the content-tree-in-transit branch September 27, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants