chore: upgrade to Node 6

[carterbs]

Resolves #291, #292.

I've added pify as an alternative to Bluebird.promisify as @malept requested. By updating ava, I was able to remove bluebird entirely.

I ran the tests in node 6.9, and they pass. I suspect the CI will still fail because it's using node 4.1. I don't know enough about the CI setup to change those values with any certainty.

[malept]

I really appreciate the work you've put into this!

the module babel-plugin-transform-async-to-module-method will not work with babel 7.

I think this is what the Babel 7 version is: https://babeljs.io/docs/en/babel-plugin-transform-async-to-generator

[carterbs]

Good suggestions. Didn't even see the readjson thing. I'll finish this up in the morning. I went with babelrc just because I thought it kept the package.json cleaner. Package.json is pretty small though so I can revert that if you want. Also appreciate notes on modern Babel practices. Haven't upgraded in about two years. :-)

…

On Thursday, February 21, 2019, Mark Lee ***@***.***> wrote: ***@***.**** requested changes on this pull request. I really appreciate the work you've put into this! the module babel-plugin-transform-async-to-module-method will not work with babel 7. I think this is what the Babel 7 version is: https://babeljs.io/docs/en/ babel-plugin-transform-async-to-generator ------------------------------ In .babelrc <#293 (comment)> : > @@ -0,0 +1,14 @@ +{ + "presets": [ + "babel-preset-node6" + ], + "plugins": [ + [ + "transform-async-to-module-method", + { + "module": "bluebird", + "method": "coroutine" + } + ] + ] +} Any particular reason why you moved the babel config out of package.json? ------------------------------ In .babelrc <#293 (comment)> : > @@ -0,0 +1,14 @@ +{ + "presets": [ + "babel-preset-node6" + ], + "plugins": [ + [ + "transform-async-to-module-method", + { + "module": "bluebird", + "method": "coroutine" + } + ] + ] +} Also, I'd replace the tabs with 2 spaces per indentation. ------------------------------ In package.json <#293 (comment)> : > "temp": "^0.8.3" }, "devDependencies": { "ava": "^0.13.0", "babel-cli": "^6.6.5", "babel-eslint": "^6.0.2", "babel-plugin-transform-async-to-module-method": "^6.7.0", - "babel-plugin-transform-runtime": "^6.6.0", - "babel-preset-es2015-node4": "^2.0.3", - "babel-preset-stage-0": "^6.5.0", + "babel-preset-node6": "^11.0.0", The modern convention is to use babel-preset-env, rather than any other babel-preset-* for compilation targets. ------------------------------ In src/fs-utils.js <#293 (comment)> : > - return false; -} +export { + createTempDir +}; Maybe rename this file to temp-utils.js? ------------------------------ In src/index.js <#293 (comment)> : > @@ -2,8 +2,8 @@ import template from 'lodash.template'; import spawn from './spawn-promise'; import asar from 'asar'; import path from 'path'; -import * as fsUtils from './fs-utils'; - +import { createTempDir } from './fs-utils'; +import { copy, readFile, rename, pathExists, writeFile } from 'fs-extra'; ⬇️ Suggested change -import { copy, readFile, rename, pathExists, writeFile } from 'fs-extra'; +import fs from 'fs-extra'; ------------------------------ In src/index.js <#293 (comment)> : > appMetadata = JSON.parse(asar.extractFile(asarFile, 'package.json')); } else { - appMetadata = JSON.parse(await fsUtils.readFile(path.join(appResources, 'app', 'package.json'), 'utf8')); + appMetadata = JSON.parse(await readFile(path.join(appResources, 'app', 'package.json'), 'utf8')); ⬇️ Suggested change - appMetadata = JSON.parse(await readFile(path.join(appResources, 'app', 'package.json'), 'utf8')); + appMetadata = await fs.readJson(path.join(appResources, 'app', 'package.json')); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#293 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APtB_qLAWTmpHPQE3sl8BbU9Cn1fciy3ks5vP0T8gaJpZM4bIpCh> .

[malept]

I went with babelrc just because I thought it kept the package.json cleaner. Package.json is pretty small though so I can revert that if you want.

Yeah, I'd just keep the babel config where it is in package.json to keep the diff small.

[carterbs]

@malept I've updated the PR description and pushed changes that resolve your suggestions.

[malept]

We're almost there! Just a few things:

• Can you update CI to run against Node 6?

windows-installer/.travis.yml

Lines 3 to 4 in 5954c8e

	node_js:
	- "4.1"

windows-installer/appveyor.yml

Lines 10 to 12 in 5954c8e

	environment:
	matrix:
	- nodejs_version: 4.3.2

Don't specify the minor/patch version, we just want to test against the latest Node 6.x version.

[carterbs]

I'm out for the rest of the day so this won't get done today. Would you be willing to update the CI files? Re: duplicate Babel. Agreed, it's bad. "inherit" wasn't working. Best I could tell, Ava will inherit from babelrc but not from package.json. If you can't tackle this stuff let me know. I can probably finish it up tomorrow morning.

…

On Friday, February 22, 2019, Mark Lee ***@***.***> wrote: ***@***.**** requested changes on this pull request. We're almost there! Just a few things: - Can you update CI to run against Node 6? https://github.com/electron/windows-installer/blob/ 5954c8e/.travis.yml#L3-L4 https://github.com/electron/windows-installer/blob/ 5954c8e/appveyor.yml#L10-L12 Don't specify the minor/patch version, we just want to test against the latest Node 6.x version. ------------------------------ In package.json <#293 (comment)> : > ], - "babel": "inherit" + "babel": { + "testOptions": { + "presets": [ + ***@***.***/preset-env" + ], + "plugins": [ + ***@***.***/plugin-transform-async-to-generator" + ] + } + } Doesn't ava's babel config read the regular Babel config? Having it duplicated isn't particularly maintainable. ------------------------------ In package.json <#293 (comment)> : > "babel-eslint": "^6.0.2", - "babel-plugin-transform-async-to-module-method": "^6.7.0", - "babel-plugin-transform-runtime": "^6.6.0", - "babel-preset-es2015-node4": "^2.0.3", - "babel-preset-stage-0": "^6.5.0", - "babel-register": "^6.7.2", "eslint": "^2.4.0" }, "engines": { "node": ">=0.4.0" I don't even know why it's 0.4.0 in the first place... ⬇️ Suggested change - "node": ">=0.4.0" + "node": ">=6.0.0" — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#293 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APtB_m3p-GjfZt6yjyZMVnq7TYjlAHvAks5vQCSwgaJpZM4bIpCh> .

[malept]

Re: duplicate Babel. Agreed, it's bad. "inherit" wasn't working. Best I could tell, Ava will inherit from babelrc but not from package.json.

Ugh. If that's the case, move the babel config to .babelrc.

If you can't tackle this stuff let me know. I can probably finish it up tomorrow morning.

I don't know what my availability is for this PR within the next day, so if you don't see any commits by tomorrow morning, feel free to do it.

[malept]

I'm just going to merge this into my branch so the tests will actually run. Thanks for all your work!

[carterbs]

Thanks for this. My weekend got super busy and I just forgot about it. Appreciate you finishing it up.

…

On Monday, February 25, 2019, Mark Lee ***@***.***> wrote: Merged #293 <#293> into asar-1.0.0. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#293 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APtB_vK1hZec3m96EwuQZeiJhV1g7eAsks5vQ4V5gaJpZM4bIpCh> .