search

fknop / hapi-pagination

Hapi plugin to handle "custom" pagination
MIT License
63 stars 36 forks source link

Hapi V17 update #63

Closed jimutt closed 6 years ago

jimutt commented 6 years ago

This PR probably should not be merged yet. But I'm putting it here for comments and review. Most work has been to modify all unit tests to use async/await. There are not that many adjustments made to the actual plugin other than making it conform to the new V17 API. It would probably be good to carry out some refactoring in order to better comply with the V17 best practices. One example is that the "reply" parameter should maybe be renamed to "h". The changes which have been made so far are very rough and the main goal has been to "just make it work" with V17.

I think there's one line which the tests doesn't cover at the moment. But all tests are at least passing and I've been testing the very basic functionality on a small v17 project and I believe it should be working by now.

When/if it gets merged you should probably make sure to squash my commits as well.

I'm by the way really new to Hapi in general, so if there's changes which looks weird there's always a possibility are that I've done something wrong. But I really need some nice header-based pagination support for a personal project of mine (which I'm planning to use Hapi V17 for), so I thought I better try to do something about it.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 99.834% when pulling 8b3f8c458813f46b9b6abb11358930b6f5b01b59 on jimutt:master into 5a78ce65db2ba9517c4e396422287ef5052f3fce on fknop:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 99.834% when pulling b9069cfd3e729a8b8d3798b555978a395bd493c0 on jimutt:master into 5a78ce65db2ba9517c4e396422287ef5052f3fce on fknop:master.

jimutt commented 6 years ago

I've renamed the "reply" occurrences to "h" for all functions, should I rename reply property in the Config to h as well?

It seems like the Exception-test "Should not continue on exception" was the one that earlier covered the "!request.response.isBoom" line. I'm not completely sure why it's not executing it now though. They seem to have changed parts of the error handling for the v17 release (for example see the "Changed how errors and takeover responses are handled in the request lifecycle" in the release notes). Could it have anything to do with that?

fknop commented 6 years ago

I don't think it's worth changing the config.

I'll take a look at the changelog.

fknop commented 6 years ago

If I understand correctly, exceptions are now handled by an internal mechanism. But it should still return a boom error with a status code of 500.

fknop commented 6 years ago

But the test seems to pass so, I think it's safe to remove that line. And there is another check just after for the status code between [200, 299].

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 87476ef80aed5eb952f0ae3d38d6ec4dc45d5e89 on jimutt:master into 5a78ce65db2ba9517c4e396422287ef5052f3fce on fknop:master.

jimutt commented 6 years ago

Removed the line and fixed all linting issues (seems like the travis build is passing now). But I haven't done anything to update the Readme though.

fknop commented 6 years ago

Thanks, I'll take care of the readme, shouldn't be too long. 👍

fknop commented 6 years ago

There you go!

https://github.com/fknop/hapi-pagination/releases/tag/2.0.0 https://www.npmjs.com/package/hapi-pagination