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

Hapi v17/18 update #1251

Merged
merged 18 commits into from Mar 19, 2019
Merged

Hapi v17/18 update #1251

merged 18 commits into from Mar 19, 2019

Conversation

Danwhy
Copy link
Contributor

@Danwhy Danwhy commented Mar 13, 2019

ref #1111

Upgrades hapi to v18:

  • upgrades all necessary hapi dependencies
  • switches reply for response toolkit (h)
  • replaces all handler callbacks with async/await

@codecov-io
Copy link

codecov-io commented Mar 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ac5786c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1251   +/-   ##
=========================================
  Coverage          ?   98.34%           
=========================================
  Files             ?      112           
  Lines             ?     1933           
  Branches          ?      540           
=========================================
  Hits              ?     1901           
  Misses            ?       32           
  Partials          ?        0
Impacted Files Coverage Δ
routes/screensaver.js 100% <100%> (ø)
routes/analytics.js 100% <100%> (ø)
routes/robot.js 100% <100%> (ø)
routes/public.js 100% <100%> (ø)
auth/validate-token.js 100% <100%> (ø)
routes/sitemap.js 100% <100%> (ø)
auth/session.js 100% <100%> (ø)
routes/random.js 100% <100%> (ø)
routes/about.js 100% <100%> (ø)
routes/home.js 100% <100%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac5786c...8cdb667. Read the comment docs.

@Danwhy Danwhy marked this pull request as ready for review March 18, 2019 17:23
Copy link
Contributor

@SimonLab SimonLab left a comment

Choose a reason for hiding this comment

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

Tested on my machine, everything looks good 🎉
Thanks @Danwhy

@SimonLab SimonLab removed their assignment Mar 19, 2019
@@ -1,7 +1,7 @@
language: node_js

node_js:
- '7.10.1'
- '8.12.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider bumping this up to LTS https://nodejs.org (v10) ? 🤔

name: 'Authentication',
register: async function (server, options) {
server.state('token', {
isSecure: false
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making this an environment variable with a default to false so that it can be configured in AWS without a code release. (it's OK for now)

const jwt = JWT.sign(session, config.JWT_SECRET);
return reply.redirect('/').state('token', jwt, {ttl: 5 * 3600 * 1000});
console.log('GOOD');
Copy link
Contributor

Choose a reason for hiding this comment

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

please consider removing these console.log('GOOD') and console.log('BAD') or making them more specific so the person reading the logs can stack-trace/debug them. 😉

const result = await elastic.search(searchOpts);
return result.hits.hits;
} catch (err) {
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

what error handler is "catching" this throw err ...?

"boom": "^6.0.0",
"catbox": "^8.0.1",
"catbox-redis": "^3.1.1",
"axios": "^0.18.0",
Copy link
Contributor

@nelsonic nelsonic Mar 19, 2019

Choose a reason for hiding this comment

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

why axios? https://www.npmjs.com/package/axios ? 💭
(used to fetch from ES/API ... but why? or why not something else?)

Copy link
Contributor

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@Danwhy tests and code updates look great! thanks! 🎉

@nelsonic nelsonic merged commit 70ea27a into master Mar 19, 2019
@nelsonic nelsonic deleted the hapi-update branch March 19, 2019 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants