Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: adds http DAG api #1930

Merged
merged 5 commits into from Mar 18, 2019
Merged

feat: adds http DAG api #1930

merged 5 commits into from Mar 18, 2019

Conversation

achingbrain
Copy link
Member

No description provided.

@achingbrain achingbrain added the status/in-progress In progress label Mar 13, 2019
@ghost ghost assigned achingbrain Mar 13, 2019
@achingbrain achingbrain force-pushed the add-http-dag-api branch 3 times, most recently from 946f455 to 6111234 Compare March 13, 2019 20:05
@alanshaw alanshaw mentioned this pull request Mar 14, 2019
50 tasks
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

❤️ 🚀 Thanks for this!

src/http/api/resources/dag.js Outdated Show resolved Hide resolved

if (key.codec === 'dag-pb' && result.value) {
if (typeof result.value.toJSON === 'function') {
result.value = result.value.toJSON()
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if this is required? JSON.stringify will call this automatically...which is what I assume happens after you pass this to h.response 😝.

I also assume that because we're not setting the Content-Type header explicitly that Hapi will infer text/plain from the string you create here and pass to h.response, instead of application/json (which is what we want).

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the HTTP API docs /dag/get should return a text/plain response body, which seems like an odd decision.

src/http/api/resources/dag.js Outdated Show resolved Hide resolved
src/http/api/resources/dag.js Show resolved Hide resolved
src/http/api/resources/dag.js Outdated Show resolved Hide resolved
src/http/api/resources/dag.js Outdated Show resolved Hide resolved
test/http-api/dag.js Outdated Show resolved Hide resolved
test/http-api/dag.js Outdated Show resolved Hide resolved
test/http-api/dag.js Outdated Show resolved Hide resolved
src/http/api/resources/dag.js Outdated Show resolved Hide resolved
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Code LGTM, just waiting for CI 👍

@ghost ghost assigned alanshaw Mar 18, 2019
@alanshaw alanshaw merged commit a033e8b into master Mar 18, 2019
@ghost ghost removed the status/in-progress In progress label Mar 18, 2019
@achingbrain achingbrain deleted the add-http-dag-api branch March 19, 2019 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants