-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[1.0] refactor node structure to hide internal fields #960
Changes from 1 commit
02d4c95
af44977
91e2fc5
730ec79
8dbc871
9c86a7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,13 +103,15 @@ fragment commentsFragment on HackerNewsItem { | |
|
||
const storyNode = { | ||
...kidLessStory, | ||
domain, | ||
order: i + 1, | ||
parent: `__SOURCE__`, | ||
type: `HNStory`, | ||
children: [...kids.kids.map(k => k.id)], | ||
parent: `__SOURCE__`, | ||
content: storyStr, | ||
mediaType: `application/json`, | ||
internal: { | ||
type: `HNStory`, | ||
mediaType: `application/json`, | ||
}, | ||
domain, | ||
order: i + 1, | ||
} | ||
|
||
// Just store the user id | ||
|
@@ -121,7 +123,7 @@ fragment commentsFragment on HackerNewsItem { | |
.update(JSON.stringify(storyNode)) | ||
.digest(`hex`) | ||
|
||
storyNode.contentDigest = contentDigest | ||
storyNode.internal.contentDigest = contentDigest | ||
|
||
createNode(storyNode) | ||
|
||
|
@@ -133,11 +135,13 @@ fragment commentsFragment on HackerNewsItem { | |
} | ||
let commentNode = { | ||
..._.omit(comment, `kids`), | ||
order: i + 1, | ||
type: `HNComment`, | ||
parent, | ||
children: [...comment.kids.map(k => k.id)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here |
||
mediaType: `application/json`, | ||
parent, | ||
internal: { | ||
type: `HNComment`, | ||
mediaType: `application/json`, | ||
}, | ||
order: i + 1, | ||
} | ||
|
||
commentNode.by = commentNode.by.id | ||
|
@@ -149,8 +153,8 @@ fragment commentsFragment on HackerNewsItem { | |
.update(nodeStr) | ||
.digest(`hex`) | ||
|
||
commentNode.contentDigest = contentDigest | ||
commentNode.content = nodeStr | ||
commentNode.internal.contentDigest = contentDigest | ||
commentNode.internal.content = nodeStr | ||
|
||
createNode(commentNode) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
const Promise = require(`bluebird`) | ||
const { | ||
GraphQLObjectType, | ||
GraphQLBoolean, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,12 +24,14 @@ export const pageSchema = Joi.object() | |
export const nodeSchema = Joi.object() | ||
.keys({ | ||
id: Joi.string().required(), | ||
contentDigest: Joi.string().required(), | ||
children: Joi.array(Joi.string()).required(), | ||
parent: Joi.string().required(), | ||
mediaType: Joi.string().required(), | ||
content: Joi.string(), | ||
type: Joi.string().required(), | ||
pluginName: Joi.string().required(), | ||
internal: Joi.object().keys({ | ||
contentDigest: Joi.string().required(), | ||
mediaType: Joi.string().required(), | ||
type: Joi.string().required(), | ||
pluginName: Joi.string().required(), | ||
content: Joi.string(), | ||
}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's added automatically by api-runner-node There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not required according to this validation, shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jquense to expand on my answer I typed on my phone as my plane took off :-) If we know which plugin created every node we can do optimizations eventually like invalidating only nodes created by a plugin when it updates, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @0x80 what's "it" here? |
||
}) | ||
.unknown() |
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.
Not part of the PR but I just happened to notice the redundant
[...]
here. It could just bechildren: kids.kids.map(k => k.id),
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.
Oh good point :-)