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

add function to tell user if dat version is out of date. Closes #777 #852

Closed
wants to merge 7 commits into from
Closed

Conversation

TimothyStiles
Copy link
Contributor

@TimothyStiles TimothyStiles commented Aug 16, 2017

Here's a proof of concept to solve issue #777 as described by @joehand and @juliangruber. Right now to run I just execute node version-check.js from the command line and it works as described in #777 aside from being run in a separate child process when dat is being used through its CLI.

I'm not familiar with child processes in node yet. What's the best way to integrate this so that it runs as a child process when dat is used via the CLI?

@TimothyStiles
Copy link
Contributor Author

Also, I do use npmview in 67c8bc4 and it fails to work when I uninstall npmview. Yet when npm is installed and being used dat's test results throw the following.

Success! All dependencies used in the code are listed in package.json

Fail! Modules in package.json not used in code: npmview

Does anyone have any idea why?

@ralphtheninja
Copy link
Contributor

@TimothyStiles I suspect that it's because dependency-check doesn't run for this particular file, it's targeting . which (I think) maps to what main is set to in package.json, in this case index.js.

@ralphtheninja
Copy link
Contributor

ralphtheninja commented Aug 16, 2017

This sounds to me like a function that should always be run, no matter which subcommand you are using. So it could be a function that returns something falsy that's invoked from within bin/cli.js. If falsey, write out warning message and process.exit(1) or something like that.

@TimothyStiles
Copy link
Contributor Author

@ralphtheninja I think the issue was that version-check.js wasn't a bin/cli.js dependency. When I added it dependency-check stopped failing.

@joehand
Copy link
Collaborator

joehand commented Aug 17, 2017

Sweet! Let's use something like latest-version instead of npmview to avoid the npm dependency.

We could use https://github.com/yeoman/update-notifier but I find the update UI on that a bit harsh. I think they take a similar approach to background notification.

@TimothyStiles
Copy link
Contributor Author

TimothyStiles commented Aug 17, 2017

Sweet! Let's use something like latest-version instead of npmview to avoid the npm dependency.

@joehand I'll switch to latest-version in the next push.

We could use https://github.com/yeoman/update-notifier but I find the update UI on that a bit harsh.

Right now the UI is an exact clone of yarn update notifier you shared in #777.

screen shot 2017-08-17 at 3 17 29 pm

@joehand
Copy link
Collaborator

joehand commented Aug 17, 2017

As far as integration, you should be able to add it to the exit UI, then somewhere (lib/archive.js ?) set state.upgradeNotify.

@TimothyStiles
Copy link
Contributor Author

As far as integration, you should be able to add it to the exit UI, then somewhere (lib/archive.js ?) set state.upgradeNotify.

@joehand how would you add it to the exit UI? Right now it's integrated by placing a call to versionCheck in the alias function in bin/cli.js.

function alias (argv) {
  var cmd = argv[0]
  versionCheck()
  if (!config.aliases[cmd]) return argv
  argv[0] = config.aliases[cmd]
  return argv
}

Would I have decompose versionCheck into two parts or could I still just call it from src/version-check.js? Sort of like:

var versionCheck = require('../version-check')
module.exports = onExit

function onExit (state, bus) {
  bus.on('exit:error', onError)
  bus.on('exit:warn', function (err) {
    onError(err, true)
  })
  process.on('SIGINT', function () {
    state.exiting = true
    bus.render()
    versionCheck()
    process.exit()
  })

  function onError (err, clear) {
    if (clear) bus.clear()
    console.error(err)
    process.exit(1)
  }
}

@ralphtheninja
Copy link
Contributor

ralphtheninja commented Aug 17, 2017

@ralphtheninja I think the issue was that version-check.js wasn't a bin/cli.js dependency. When I added it dependency-check stopped failing.

Aye, which is what I meant, I think :)

@joehand
Copy link
Collaborator

joehand commented Aug 18, 2017

Ya, the main thing I'd like is for it to print out at the end of a command, not the top. So it seems like the exit is the easiest place to hook into that.

onExit is called at the beginning of any command, so you should be able to check the version and then set the state if it is outdated:

function onExit (state, bus) {
  versionCheck(function (err, outdated) {
     state.versionOutdated = outdated
  })

  // ... other stuff
}

We have to figure out where to render that... I may make a global ui element. I've been using archive.js for that but we should have something for stuff like this to go into. Something like this in archive.js may work for now:

var onExit = `
  ${state.versionOutdated ? 'TODO: outdated warning' : ''}
  Exiting the Dat program...
`
${state.exiting ? onExit : chalk.dim('Ctrl+C to Exit')}


module.exports = function () {
latestVersion('dat').then(function (version) {
fs.writeFile('../.dat-version', version, function (err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to write to a in the home-dir (or somewhere that'll be the same no matter where Dat is run). Maybe write to ~/.dat/cli-version.txt (we use os-home module elsewhere)?

})
})

if (fs.existsSync('../.dat-version')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's avoid sync functions in this one so we don't block anything else. You can use fs.stat instead.

})

if (fs.existsSync('../.dat-version')) {
var newestVersion = fs.readFileSync('../.dat-version', 'utf8')
Copy link
Collaborator

Choose a reason for hiding this comment

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

fs.readFile(name, cb)

@joehand joehand added this to the 13.11 milestone Mar 8, 2018
@joehand joehand closed this Mar 8, 2018
@joehand joehand reopened this Mar 8, 2018
@joehand
Copy link
Collaborator

joehand commented Mar 27, 2018

see updated PR; #968

@joehand joehand closed this Mar 27, 2018
joehand added a commit that referenced this pull request Apr 12, 2018
* added npmview dependency for latest version checking.

* added proof of concept for version checker.

* ran standard --fix to fix style so tests can pass.

* made version-check exportable.

* added version check to 'bin/cli.js'.

* removed npmview and added latest-version to package.json

* switched npmview with latest-version in version-check.js

* use update notifier for upgrade message
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 this pull request may close these issues.

3 participants