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

refactor: replace inherited logging, exit utils in commands.functions… #3347

Merged
merged 3 commits into from
Sep 16, 2021

Conversation

tinfoil-knight
Copy link
Contributor

@tinfoil-knight tinfoil-knight commented Sep 16, 2021

- Summary

#3028 was too large for review and is being broken down.

This PR is a follow-up on #3247 and uses the separate logging and process control utilities created in it to remove the inherited ones.

This PR focuses specifically on the src/commands/functions folder. Utilities outside this folder have been changed as less as possible to avoid growing the diff size in a single PR.

- Test plan

npx ava --verbose --serial ran with 1 failing test: serving-functions
See:
image

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Sep 16, 2021
@@ -243,7 +243,7 @@ const ensureFunctionDirExists = async function (context) {
})

log(`${NETLIFYDEVLOG} functions directory ${chalk.magenta.inverse(functionsDirHolder)} updated in site settings`)
} catch (error) {
} catch (_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch (_) {
} catch {

@@ -279,7 +279,7 @@ const downloadFromURL = async function (context, flags, args, functionsDir) {

try {
mkdirRecursiveSync(fnFolder)
} catch (error) {
} catch (_) {
Copy link
Contributor

@ehmicky ehmicky Sep 16, 2021

Choose a reason for hiding this comment

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

Suggested change
} catch (_) {
} catch {

@@ -365,7 +365,7 @@ const installDeps = async ({ functionPackageJson, functionPath, functionsDir })
const functionPackageLock = path.join(functionPath, 'package-lock.json')

fs.unlinkSync(functionPackageLock)
} catch (error) {
} catch (_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch (_) {
} catch {

@@ -270,7 +271,7 @@ const tryParseJSON = function (jsonString) {
if (parsedValue && typeof parsedValue === 'object') {
return parsedValue
}
} catch (error) {}
} catch (_) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch (_) {}
} catch {}

Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks @tinfoil-knight, this looks great!
I just added a couple of small suggestions.

@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Sep 16, 2021

Hey @ehmicky, I've implemented the suggestions. Thanks for the review.

The catch(_) pattern is there in a lot of places in the repo. Should I create a separate PR and fix those too?

@ehmicky
Copy link
Contributor

ehmicky commented Sep 16, 2021

The catch(_) pattern is there in a lot of places in the repo. Should I create a separate PR and fix those too?

If you'd like to, sure! Netlify CLI supports only Node.js >=10.18.0, so optional catch clauses are supported.

@ehmicky ehmicky added the automerge Add to Kodiak auto merge queue label Sep 16, 2021
@ehmicky ehmicky merged commit 8c87871 into netlify:main Sep 16, 2021
@tinfoil-knight tinfoil-knight deleted the log-util-functions branch September 24, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants