-
Notifications
You must be signed in to change notification settings - Fork 127
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/doctypes #6
Conversation
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've added a couple of comments. Nothing crucial, just recommendations.
@@ -65,7 +66,7 @@ class Document extends EventEmitter { | |||
} | |||
|
|||
async change (newContent: any, apiUrl: string): Promise<boolean> { | |||
const { docId, state } = await fetchJson(apiUrl + '/change' + this.id, { content: newContent }) | |||
const { state } = await fetchJson(apiUrl + '/change' + this.id, { content: newContent }) |
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.
Let's normalize the URL concatenation in order to avoid multiple slashes if the apiUrl
already ends with /
. Maybe a separate utility function.
this.app.post(toApiPath('/change/ceramic/:doctype/:cid'), this.change.bind(this)) | ||
this.app.get(toApiPath('/show/ceramic/:cid'), this.show.bind(this)) | ||
this.app.get(toApiPath('/state/ceramic/:cid'), this.state.bind(this)) | ||
this.app.post(toApiPath('/change/ceramic/:cid'), this.change.bind(this)) | ||
const server = this.app.listen(7007, () => { |
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.
We can put a daemon port
in the configuration file
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.
Yep, that should be configurable once we have a config file!
console.log(JSON.stringify(doc.content, null, 2)) | ||
try { | ||
const doc = await ceramic.createDocument(content, doctype, { onlyGenesis }) | ||
console.log(doc.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.
We can introduce a specific logger (e.g. winston
) instead of directly using console.log
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.
Definitely, created an issue here: #8
console.log(doc.id) | ||
console.log(JSON.stringify(doc.content, null, 2)) | ||
} catch (e) { | ||
console.error(e) |
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.
Maybe process.exit()
with a specific code?
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 makes sense!
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.
@@ -27,6 +27,20 @@ export interface InitOpts { | |||
|
|||
const deepCopy = (obj: any): any => JSON.parse(JSON.stringify(obj)) |
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.
Really useful function for cloning. Maybe we can put it in the common utility class?
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 I think there are more of these useful functions that we can pull out to a utils module.
const doc = await this.ceramic.createDocument(genesis, doctype, { onlyGenesis }) | ||
res.json({ docId: doc.id, state: doc.state }) | ||
} catch (e) { | ||
res.json({ error: e.message }) |
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.
Do we need to handle Response
error status codes?
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, right now this isn't really using best practices for express or rest apis in general. We should make sure to do this properly!
Already pushed the first commit to master 馃檲
This adds support for the signed doctypes 3id, and tile in the cli.