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.

Sign up for GitHub

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: gracefully fail Edge Functions in case of deno binary issues #4685

Merged
merged 20 commits into from
Jun 23, 2022
Merged

fix: gracefully fail Edge Functions in case of deno binary issues #4685

Changes from 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
578aad6
fix: gracefully fail in case deno binary issue
jackiewmacharia Jun 9, 2022
b5ed9ce
fix: fix linting
jackiewmacharia Jun 9, 2022
06b3f64
fix: fix typo
jackiewmacharia Jun 9, 2022
3fcb21c
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 9, 2022
48c8101
fix: update error text
jackiewmacharia Jun 9, 2022
aae01e0
fix: move error message to edge-bundler
jackiewmacharia Jun 14, 2022
5066fba
fix: log error
jackiewmacharia Jun 14, 2022
86254fe
chore: update contributors field
jackiewmacharia Jun 14, 2022
de3c686
fix: get spinner stae from error object
jackiewmacharia Jun 15, 2022
2de6cba
Merge branch 'fix/gracefully-fail-in-case-deno-binary-issue' of githu…
jackiewmacharia Jun 15, 2022
9b399e3
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 15, 2022
b6dee4a
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 15, 2022
6c2f344
chore: move error handling to initializeProxy and use custom error lo…
jackiewmacharia Jun 15, 2022
1f146ae
fix: reduce functionality covered by try/catch
jackiewmacharia Jun 16, 2022
9474027
fix: clean up catch block
jackiewmacharia Jun 17, 2022
257cd54
fix: fix linting
jackiewmacharia Jun 17, 2022
b41906a
chore: optional error value for onAfterDownload
jackiewmacharia Jun 17, 2022
72be888
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 17, 2022
064dd07
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 22, 2022
df7f247
Merge branch 'main' into fix/gracefully-fail-in-case-deno-binary-issue
jackiewmacharia Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/lib/edge-functions/proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// @ts-check
const { relative } = require('path')
const { cwd, env } = require('process')
const { format } = require('util')

const getAvailablePort = require('get-port')
const { v4: generateUUID } = require('uuid')

const { NETLIFYDEVERR, NETLIFYDEVWARN, chalk, log } = require('../../utils/command-helpers')
const { NETLIFYDEVERR, NETLIFYDEVWARN, chalk, error, log } = require('../../utils/command-helpers')
const { getGeoLocation } = require('../geo-location')
const { getPathInProject } = require('../settings')
const { startSpinner, stopSpinner } = require('../spinner')
Expand All @@ -22,8 +23,11 @@ const LOCAL_HOST = '127.0.0.1'
const getDownloadUpdateFunctions = () => {
let spinner

const onAfterDownload = () => {
stopSpinner({ spinner })
/**
* @param {Error | null} error_
*/
const onAfterDownload = (error_) => {
stopSpinner({ error: Boolean(error_), spinner })
}

const onBeforeDownload = () => {
Expand Down Expand Up @@ -74,16 +78,27 @@ const initializeProxy = async ({
projectDir,
})
const hasEdgeFunctions = userFunctionsPath !== undefined || internalFunctions.length !== 0
let hasServerError = false

return async (req) => {
if (req.headers[headers.Passthrough] !== undefined || !hasEdgeFunctions) {
if (req.headers[headers.Passthrough] !== undefined || !hasEdgeFunctions || hasServerError) {
return
}

const [geoLocation, { registry }] = await Promise.all([
getGeoLocation({ mode: geolocationMode, offline, state }),
server,
])
let promiseResult

try {
Copy link
Member

@eduardoboucas eduardoboucas Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry if this wasn't clear in my previous comment, but I was trying to suggest that you only wrap the Promise.all call in the try/catch, not the entire block. Otherwise we're back to the problem of wrapping too much code and potentially swallowing errors.

Something like:

let ops

try {
  ops = await Promise.all([
    getGeoLocation({ mode: geolocationMode, offline, state }),
    server,
  ])
} catch (err) {
  error(err, { exit: false })

  hasServerError = true

  return
}

const [geoLocation, { registry }] = ops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaah I see, done.

promiseResult = await Promise.all([getGeoLocation({ mode: geolocationMode, offline, state }), server])
} catch (error_) {
error(error_ instanceof Error ? error_ : format(error_), { exit: false })
jackiewmacharia marked this conversation as resolved.
Show resolved Hide resolved
hasServerError = true
}

if (promiseResult === undefined) {
jackiewmacharia marked this conversation as resolved.
Show resolved Hide resolved
return
}

const [geoLocation, { registry }] = promiseResult

// Setting header with geolocation.
req.headers[headers.Geo] = JSON.stringify(geoLocation)
Expand Down