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

Usage without callback never sets notifier.update #21

Closed
Bartvds opened this issue Dec 14, 2013 · 4 comments
Closed

Usage without callback never sets notifier.update #21

Bartvds opened this issue Dec 14, 2013 · 4 comments

Comments

@Bartvds
Copy link
Contributor

Bartvds commented Dec 14, 2013

When using update-notifier without callback the notifier.update will never set with the update information:

var updateNotifier = require('update-notifier');
var notifier = updateNotifier({
    packageName: 'update-notifier-tester',
    packageVersion: '0.0.2'
});
if (notifier.update) {
    // never happens
}

Same setting with a callback works just fine (the callback gets the update object).

The config file in my profile is created when I use without callback, but it only contains this:

optOut: false
lastUpdateCheck: 1386985496657

I'm on windows using node v0.10.22.

@sindresorhus
Copy link
Member

Whenever you initiate the update notifier and it's not within the interval threshold, it will asynchronously check with NPM in the background for available updates, then persist the result. The next time the notifier is initiated the result will be loaded into the .update property.

It's only available on the second run, since fetches it in the background in the first run, then checks for it in the second run.

@Bartvds
Copy link
Contributor Author

Bartvds commented Dec 16, 2013

Yea, that 2-step usage is why I wanted to use it in this way. My app can finish it's own stuff quite quickly sometimes so this is a nice approach to not let the uses wait on a slow npm call.

I made two isolated demos (well, they are the snippets from the README mostly)

First one works fine but waits on slow npm call:

var updateNotifier = require('update-notifier');
var notifier = updateNotifier({
    packageName: 'update-notifier-tester',
    packageVersion: '0.0.1',
    callback: function (err, update) {
        if (err) {
            console.log(err);
        }
        else {
            console.log(update);
        }
    }
});

But this doesn't work:

var updateNotifier = require('update-notifier');
var notifier = updateNotifier({
    packageName: 'update-notifier-tester',
    packageVersion: '0.0.1'
});

if (notifier.update) {
    console.log(notifier.update);
}
else {
    console.log('no info');
}

I can run that a few times, even wait a minute between runs (to allow sub-process to finish) but it never shows the info.

If I look at the tests I see this usage is not really tested: they all use the suite's done as callback (I see you at #5 😺)

Note I'm on Windows (yesyes): if it is really supposed to work as-is it could be that.

If it is just a forgotten feature I might look into it myself later to re-able it.

@Bartvds
Copy link
Contributor Author

Bartvds commented Jan 26, 2014

I had time to look into this: It seem the child-process is not finishing it's work: the main process kills the forked process when the main is done earlier then the child.

If I add an extra time-out the forked process has time enough to finish, so at the second run of the script the notifier.update will be set.

I can also remove the timeout and instead keep the child-process:

// cp.unref();
// cp.disconnect();

When I run the script I see a noticeable pause, but then second run there will be the expected notifier.update.

var updateNotifier = require('update-notifier');
var notifier = updateNotifier({
    packageName: 'update-notifier-tester',
    updateCheckInterval: 1,
    packageVersion: '0.0.1'
});

if (notifier.update) {
    console.log('update!');
    console.log(notifier.update);
}
else {
    console.log('no update :(');
}

// keep alive
setTimeout(function(){
    console.log('bye!');
}, 5000);

I did find a possible fix: if I modify update-notifier to use spawn instead of fork and use the detached option things behave as expected:

cp = spawn(process.execPath, [__dirname + '/check.js', JSON.stringify(this.options)], {
    detached: true,
    stdio: ['ignore', 'ignore', 'ignore']
})
cp.unref();

What do you think?

@satazor
Copy link
Contributor

satazor commented Jul 9, 2014

I guess this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants