search

nodejs / installer

Electron based installer for Node.js.
Other
194 stars 36 forks source link

Updated install function #31

Closed delvedor closed 7 years ago

delvedor commented 8 years ago

I have updated the install function removing the update parameter, because the new UI manages in a different way the refresh after the installation and because in my opinion install should only do his job, everything else can be done in the callback.

I also updated the callback inside check-node.js, because was passing the error as second argument.

delvedor commented 8 years ago

@mikeal @addaleax @JoeDoyle23 I think this can be merged, is just a little fix to make the install function compliant with the UI code :)

addaleax commented 8 years ago

Mh, one reason why I suggested that the changes to lib/check-node.js be in a different commit/different PR would be that that looks like an obviously correct and logically independent bugfix, and I would feel pretty comfortable merging that rather swiftly.

The rest of the changes… not sure, maybe I lack a global look at things (and I haven’t tried the current master of the installer myself, will do when I have the time). It doesn’t really look useless to me to have a way of notifying the UI of the “Download complete” situation.

delvedor commented 8 years ago

Ok, you are right about the different commit for lib/check-node.js, I'll update it.

Regarding the update function at the moment during download/installation the UI does not change, it has a spinner and waits for the callback of success/fail of the entire process. That's why I'm proposing to remove that function. If you feel that the 'Download Finished' must be notified in the UI we can discuss about it :)

addaleax commented 7 years ago

If you feel that the 'Download Finished' must be notified in the UI we can discuss about it :)

Not necessarily. I find progress bars / status indicators for things like downloads extremely useful sometimes, especially when I’m on a slow connection.

I see no need to block this PR on that, but maybe something like an EventEmitter for notifying the UI would give some flexibility in that direction without forcing the UI to display information in a specific way.

(sorry for not following up sooner!)

delvedor commented 7 years ago

I like the idea of the event emitter could be even simpler to implement :)

addaleax commented 7 years ago

@delvedor If you want to, feel free to go ahead and implement that. On a more general note, it might be worthwhile to think about separating the installer logic from the UI and turning the former into its own package?

delvedor commented 7 years ago

I'll definitely continue to develop this :)

Regarding the separate package for the installer I agree with you, put it in a separate package could be a good idea.