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

fix: live tunnel arch issue #4064

Merged
merged 10 commits into from
Jan 19, 2022
18 changes: 4 additions & 14 deletions bin/run
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env node
/* eslint-disable promise/prefer-await-to-then,promise/prefer-await-to-callbacks,eslint-comments/disable-enable-pair */
const process = require('process')
const { format } = require('util')
erezrokah marked this conversation as resolved.
Show resolved Hide resolved

const updateNotifier = require('update-notifier')

Expand All @@ -18,23 +19,12 @@ if (require.main === module) {
pkg,
updateCheckInterval: UPDATE_CHECK_INTERVAL,
}).notify()
} catch (error) {
} catch (error_) {
console.log('Error checking for updates:')
console.log(error)
console.log(error_)
}

/** @type {Error} */
let caughtError

const program = createMainCommand()

program.parseAsync(process.argv).catch((error) => {
caughtError = error
})

// long running commands like dev server cannot be caught by a post action hook
// they are running on the main command
process.on('exit', () => {
program.onEnd(caughtError)
})
program.parseAsync(process.argv).catch((error_) => program.onEnd(error_))
}
90 changes: 89 additions & 1 deletion npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@
"mock-fs": "^5.1.2",
"mock-require": "^3.0.3",
"p-timeout": "^4.0.0",
"proxyquire": "^2.1.3",
"seedrandom": "^3.0.5",
"serialize-javascript": "^6.0.0",
"sinon": "^12.0.0",
Expand Down
14 changes: 8 additions & 6 deletions src/commands/base-command.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,14 @@ class BaseCommand extends Command {
debug(`${this.name()}:onEnd`)(`Status: ${status}`)
debug(`${this.name()}:onEnd`)(`Duration: ${duration}ms`)

await track('command', {
...payload,
command: this.name(),
duration,
status,
})
try {
await track('command', {
...payload,
command: this.name(),
duration,
status,
})
} catch {}

if (error_ !== undefined) {
error(error_ instanceof Error ? error_ : format(error_), { exit: false })
Expand Down
50 changes: 45 additions & 5 deletions src/lib/exec-fetcher.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
// @ts-check
const path = require('path')
const process = require('process')
const { stringify } = require('querystring')
Copy link
Contributor

Choose a reason for hiding this comment

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

[sand] What are your thoughts on using new URL() and url.searchParams.set() instead?
This is mostly equivalent, except it uses the DOM API instead of Node.js core API. What are your thoughts on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea why not I don't have a preference to be honest -> is there any benefit?

Copy link
Contributor

@ehmicky ehmicky Jan 17, 2022

Choose a reason for hiding this comment

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

If we had more code to share with the front-end, it would be better. And Node.js has been moving in that direction lately of using more and more the DOM API, as opposed to to its own core Node.js modules (URI, AbortSignal, EventTarget, globalThis, ES modules, Blob, ReadableStream/WritableStream/TransformStream, Worker, Web Crypto, performance, structuredClone), which seems a good thing at a higher level, and is also closer to the direction Deno is heading.

However, this is more of a conceptual benefit in this case, since we do not intend to run this file anywhere but on a terminal in this case, so either way could work. πŸ‘

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized this too: querystring is marked as "legacy" in the Node.js documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll change it for you ;)


const { fetchLatest, fetchVersion, newerVersion, updateAvailable } = require('gh-release-fetch')
const isExe = require('isexe')
const terminalLink = require('terminal-link')

// cannot directly import from ../utils as it would create a circular dependency.
// the file `src/utils/live-tunnel.js` depends on this file
const { NETLIFYDEVWARN, log } = require('../utils/command-helpers')
const { NETLIFYDEVWARN, chalk, error, log } = require('../utils/command-helpers')
const execa = require('../utils/execa')

const isWindows = () => process.platform === 'win32'
Expand Down Expand Up @@ -62,27 +64,65 @@ const shouldFetchLatestVersion = async ({ binPath, execArgs, execName, latestVer
latestVersion,
})
return outdated
} catch (error) {
} catch (error_) {
if (exists) {
log(NETLIFYDEVWARN, `failed checking for new version of '${packageName}'. Using existing version`)
return false
}
throw error
throw error_
}
}

/**
* Tries to get the latest release from the github releases to download the binary.
* Is throwing an error if there is no binary that matches the system os or arch
* @param {object} config
* @param {string} config.destination
* @param {string} config.execName
* @param {string} config.destination
* @param {string} config.extension
* @param {string} config.packageName
* @param {string} [config.latestVersion ]
*/
const fetchLatestVersion = async ({ destination, execName, extension, latestVersion, packageName }) => {
const win = isWindows()
const platform = win ? 'windows' : process.platform
const pkgName = `${execName}-${platform}-${process.arch}.${extension}`

const release = {
repository: getRepository({ packageName }),
package: `${execName}-${platform}-amd64.${extension}`,
package: pkgName,
destination,
extract: true,
}

const options = getOptions()
await (latestVersion ? fetchVersion({ ...release, version: latestVersion }, options) : fetchLatest(release, options))
const fetch = latestVersion
? fetchVersion({ ...release, version: latestVersion }, options)
: fetchLatest(release, options)

try {
await fetch
} catch (error_) {
if (typeof error_ === 'object' && 'statusCode' in error_ && error_.statusCode === 404) {
const qs = stringify({
assignees: '',
labels: 'type: bug',
template: 'bug_report.md',
title: `${execName} is not supported on ${platform} with CPU architecture ${process.arch}`,
})
const issueLink = terminalLink('Create a new CLI issue', `https://github.com/netlify/cli/issues/new?${qs}`)

error(`The operating system ${chalk.cyan(platform)} with the CPU architecture ${chalk.cyan(
process.arch,
)} is currently not supported!

Please open up an issue on our CLI repository so that we can support it:
${issueLink}`)
}

error(error_)
}
}

module.exports = { getExecName, shouldFetchLatestVersion, fetchLatestVersion }
Loading