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.

Sign up for GitHub

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

Support on close without the instance object #76

Merged
merged 5 commits into from
Oct 5, 2018
+49 −5
Merged

Support on close without the instance object #76

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7bd9822
Support on close without the instance object
cemremengu Sep 9, 2018
9e4cec2
Added unit test
cemremengu Sep 9, 2018
c1b841f
if it is 0 there is no point in calling it with this as first argument
cemremengu Sep 10, 2018
23b9e7b
Updated tests
cemremengu Sep 10, 2018
6ab5470
Update 1 param behavior, make tests pass
cemremengu Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions boot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ Boot.prototype.ready = function (func) {
})
}

function noop () {}
function noop () { }

function callWithCbOrNextTick (func, cb, context) {
context = this._server
Expand Down Expand Up @@ -350,8 +350,11 @@ function closeWithCbOrNextTick (func, cb, context) {
function encapsulateTwoParam (func, that) {
return _encapsulateTwoParam.bind(that)
function _encapsulateTwoParam (context, cb) {
if (func.length === 0 || func.length === 1) {
func(this)
if (func.length === 0) {
func()
process.nextTick(cb)
} else if (func.length === 1) {
func(cb)
cemremengu marked this conversation as resolved.
Show resolved Hide resolved
process.nextTick(cb)
Copy link
Member

Choose a reason for hiding this comment

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

You were right. This needs to change to:

func(cb)

Remove the  process.nextTick thing.

Copy link
Contributor Author

@cemremengu cemremengu Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Tests are failing with that change onClose arguments - fastify encapsulation test case: not ok child test left in queue

function encapsulateTwoParam (func, that) {
  return _encapsulateTwoParam.bind(that)
  function _encapsulateTwoParam (context, cb) {
    if (func.length === 0) {
      func()
      process.nextTick(cb)
    } else if (func.length === 1) {
      func(cb)
    } else {
      func(this, cb)
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe tests need to be updated to call done() ?

} else {
func(this, cb)
Expand Down
14 changes: 14 additions & 0 deletions test/close.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,20 @@ test('close without a cb', (t) => {
app.close()
})

test('close without an instance', (t) => {
t.plan(2)

const app = boot()

app.onClose((done) => {
t.ok('called')
})

app.close(() => {
t.pass('closed')
Copy link
Member

Choose a reason for hiding this comment

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

you might want to:

  1. use a function ( ) {} so you have access to the arguments object
  2. place an assertion that checks that arguments.length === 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like length is always 1 as {0 : null} so I can check like:

    t.is(arguments[0], null)
    t.is(arguments[1], undefined)

Is that alright?

Copy link
Member

Choose a reason for hiding this comment

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

then there is possibly something off with your code, as https://github.com/mcollina/avvio/pull/76/files#diff-fe60a8aed4b3103661f9d88361346b0dR353 should be triggered.

})
})

test('close passing not a function', (t) => {
t.plan(1)

Expand Down