search

bleupen / halacious

a better HAL processor for Hapi
MIT License
108 stars 22 forks source link

Hapi v17 Support #103

Open timcosta opened 6 years ago

timcosta commented 6 years ago

Overview

If you are not aware yet, Hapi v17 is making the transition from callbacks to async/await, as well as deprecating some other rarely used functionality. This is a breaking change that may make your plugin no longer compatible with the Hapi API.

Changelog

Draft release notes can be found here: https://github.com/hapijs/hapi/issues/3658

Target Release

The target release date for v17 is the beginning of November.

Tasks

Notes

pkoniu commented 6 years ago

FYI in the following weeks I'll try to upgrade halacious to support Hapi v17.

bleupen commented 6 years ago

Great! Our organization wont be able to upgrade to v17 for quite some time. It will require a massive refactor of our app for little benefit (we are still on node v6 so no async/await for us yet). Thus, updating halacious hasnt been a top priority project for me.

However, I am definitely interested in keeping the library current and taking the opportunity to simplify and clean up the code!

johannessjoberg commented 6 years ago

Any progress on this?

alexb-uk commented 6 years ago

Hi all,

I've just spent last 24hrs updating Halacious to Node 8 and HAPI 17. The tests are all passing however mocha is getting stuck before exiting, I've never used mocha before so I've not spent much time looking into this, I suspect HAPI server isn't exiting.

I've only had time to test the plugin against one operation in one of my projects but so far so good.

If anyone would like to give it a try and give me some feedback that would be great.

See: https://github.com/alexb-uk/halacious

Package.json: "dependencies": { ... "halacious": "github:alexb-uk/halacious#master", ... },

Note: this is extremely rough around the edges and needs quite a bit more work / testing to be trustworthy in production. Not to mention actually leverage Node v8 improvements e.g. async/await & Promises etc.

Cheers

Alex

P.S. apologises for applying my own ESLint rules and auto-fixing the code. It was by far the quickest way I could think of to shorten the syntax.

travi commented 6 years ago

as of mocha v4 it no longer force exits. it looks like you updated mocha, so i'd bet thats the issue.

the quick fix is to add the --exit flag to the test command, but stopping the server properly would be the best fix. i'm not familiar enough with these tests to know how big of a change that would involve, so probably a question for @bleupen to decide if its worth going that far for this upgrade.

alexb-uk commented 6 years ago

@travi ah yes I meant to mention in my previous post that I'd taken the decision to upgrade all 3rd party libs while I was at it.

Given the effort required to get everything working on Node 8 and HAPI 17 it seemed like a good opportunity :)

wms commented 6 years ago

@alexb-uk You probably need to explicitly stop the server. Take https://github.com/alexb-uk/halacious/blob/master/test/plugin-spec.js#L283 for example - you'd want to add .then(() => server.stop()) to the end of the Promise chain.

Further, you could probably clean up all those long Promise chains with await, seeing as you're already inside an async function.

alexb-uk commented 6 years ago

Thanks @warrenseymour, sorry for the delay in response been a manic few weeks.

I've fixed the tests as the issue was indeed the HAPI server not being stopped.

Hopefully somebody can confirm if the file /test/test-server.js is used. I could not find any references within the project and it was also causing the tests to hang. Hence rather than fixing it I've removed it.

Cheers

P.S. The await/async is definitely a good shout but I need to build myself up to rewriting the 2400+ lines of code haha

travi commented 5 years ago

what is the state of this? @alexb-uk is your fork in a state that could have a PR opened against this project with the needed changes?

alexb-uk commented 5 years ago

Hi @travi I've been using it in a number production REST services (albeit only a fairly small set of users at the moment) not long after I forked the project.

I've used a few versions of HAPI v17 with it, currently on latest 17.5.5.

I've not had chance to tidy the promise code / leverage any other ES2015+ improvements. If there is anything people feel strongly about I can try and find some time prior to putting in a PR.

So in short, yeah I think its good for a PR but open to comments / suggestions.

alexb-uk commented 5 years ago

Oh and I'll have to check what changes were in this project for v5.0.1 as they've not been merged into my fork.

EDIT: Ignore this comment, it's too early in the morning for me, v5.0.1 was out last year not this year so I do have those changes. Oops :)

travi commented 5 years ago

So in short, yeah I think its good for a PR but open to comments / suggestions.

I think that'd be great, just to get this back to compatible with the latest versions of hapi, especially after this article was published.

I've not had chance to tidy the promise code / leverage any other ES2015+ improvements. If there is anything people feel strongly about I can try and find some time prior to putting in a PR.

seems like any additional changes could wait until after getting to the point of havi v17 support, but thats just my vote

alexb-uk commented 5 years ago

@travi just a heads-up that I've submitted a PR #104

johannessjoberg commented 4 years ago

Bump! ☝️