Skip to content

Conversation

@davidgljay
Copy link

@davidgljay davidgljay commented Nov 2, 2016

What does this PR do?

Creates a stream endpoint that returns a static json object and adjusts the frontend to expect data of that format.

How do I test this PR?

  1. npm run build
  2. npm start
  3. Visit http://localhost:3000/client/coral-embed-stream/samplearticle.html
  4. The page should render appropriately (though you won’t be able to post comments.)
  5. Your server logs should say ‘Stream endpoint has been hit with asset_id assetTest’. (This is temporarily there for verification.)

Notes

  • I went ahead and added an npm build script for efficiency, someone with more of a handle on the architecture (i.e. @wyattjoh or @okbel) should feel free to refactor.
  • I went ahead and set up a path for hosting static built files. Same as above, feel free to refactor.

comments: rootComments
}))

Object.keys(childComments).reduce((prev, key) => {
Copy link

Choose a reason for hiding this comment

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

From @riley: since this block doesn't return things by keys, we should use a for each here.


router.get('/', (req, res) => {
console.log('Stream endpoint has been hit with asset_id ', req.query.asset_id)
res.setHeader('Content-Type', 'application/json');
Copy link

Choose a reason for hiding this comment

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

res.json() will take care of the header and stringify for you.


const router = express.Router();

router.get('/', (req, res) => {
Copy link

Choose a reason for hiding this comment

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

Add a third param called next here, which can be used to pass errors down the first error handler it finds down the chain.

See: https://expressjs.com/en/guide/error-handling.html

Copy link

Choose a reason for hiding this comment

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

we'll add it later. I think linting might fail if there are unused vars, and this is a mock for now.

davidgljay and others added 4 commits November 3, 2016 10:52
And fixing a few small issues with item posting.
# Conflicts:
#	routes/api/index.js
res.json is not a Promise.
Copy link

@riley riley left a comment

Choose a reason for hiding this comment

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

lgtm 💯

@riley riley merged commit cc14e73 into master Nov 4, 2016
@riley riley deleted the mock-stream-endpoint branch November 4, 2016 16:58
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.

4 participants