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

Migrate to node 8 and async/await #12

Merged
merged 1 commit into from Dec 5, 2017

Conversation

@corbinu
Copy link
Contributor

corbinu commented Nov 28, 2017

Migrates oppsy to node 8 and asyc/await. This is a requirement for good supporting hapi v17

else {
results.host = host;
this.emit('ops', results);
const tasks = [];

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

Seems kind of wasteful to iterate over the object and push into an array after each interval. Can we change something we so don't need to do that every time the interval elapses?

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

The issue here is the task tests would need to be totally rewritten as they change _tasks to a different set of tasks. I will probably come back to this but it is basically what Items was doing before.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 29, 2017

Author Contributor

do you still want this changed? I can go back to using Items if it makes you feel better for this to be hidden away. Really don't want a bunch of test rewrites to block release.

host
};

for (const taskName in this._tasks) {

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

Especially since we end up doing it twice down here.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

See above

@@ -17,27 +16,35 @@ class Network {
this._httpAgents = [].concat(httpAgents || Http.globalAgent);
this._httpAgents = [].concat(httpsAgents || Https.globalAgent);

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

I think this should be this._httpsAgents. It's an old bug.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

Fixed will be in the next commit once the other changes are settled

};

this.concurrents = (callback) => {

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

We're dropping this because hapi no longer supports multiple connections, correct?

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

Correct!

@@ -2,8 +2,16 @@

exports.makeContinuation = (predicate) => {

return (callback) => {
return () => {

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

You changed the implementation of makeContinuation but didn't change any of the call sites. Also, since this no longer makes a continuation, you should probably either change the name or just remove it and do the promise in-line.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

I am not sure which call sites you mean. These aren't exported only used internally in _.tasks. Removing the dependency on Items and just using Promise.all was the purpose here.

Also maybe I have the naming wrong but my understanding was it is called a continuation as it continues after the next tick which is still true so not sure what you want me to change this to but happy to take suggestions.

This comment has been minimized.

Copy link
@arb

arb Nov 28, 2017

Contributor

A continuation is a callback essentially. This is making something into a promise so you are essentially promisifying it so Promise.all works. makePromise as a possible name.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 28, 2017

Author Contributor

Ok so your saying we don't need the nextTick anymore or that it should be something like "resolveNextTick"?

This comment has been minimized.

Copy link
@arb

arb Nov 29, 2017

Contributor

It can be named promisify or something simar.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 29, 2017

Author Contributor

@arb I think you need to look at the code again. If it was just a promisify I would be using the util.promisify in node.

The point of the original function was to make sure the callback didn't return till the "nextTick". Either that is no longer needed or it should be named based on what it does.

This comment has been minimized.

Copy link
@arb

arb Nov 30, 2017

Contributor

We are bikeshedding, just change the name to something else. It's not creating a continuation anymore. The fact that it happens on next tick is an implementation detail and doesn't need to be reflected in the name.

This comment has been minimized.

Copy link
@corbinu

corbinu Nov 30, 2017

Author Contributor

Yes it does to make it clear to people it isn't just a promisify. I was simply trying to determine if I could just drop the nextTick semantics. Clearly the answer is no so I will name it appropriately. Can you please take a look at my other comments thanks!

This comment has been minimized.

Copy link
@arb

arb Dec 2, 2017

Contributor

The nextTick is there so that everything inside tasks is async. That's what it's there for. The only current changes required are to rename this function and the https bug mentioned above.

@corbinu

This comment has been minimized.

Copy link
Contributor Author

corbinu commented Dec 3, 2017

@arb Should be all set unless you see anything else. Thanks for all your help!

@arb
arb approved these changes Dec 5, 2017
@arb arb added this to the 2.0.0 milestone Dec 5, 2017
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 5, 2017

Are you planning on making the http/https updates on a subsequent PR?

@arb arb merged commit 5ce0123 into hapijs:master Dec 5, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arb arb self-assigned this Dec 5, 2017
@corbinu

This comment has been minimized.

Copy link
Contributor Author

corbinu commented Dec 5, 2017

The update is in there just rebased the changes to make it a little cleaner. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.