-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for refactoring! I tested locally and found some bugs. See my comments below.
src/utils/build.js
Outdated
'node_modules', | ||
constants.PLATFORM_PACKAGE | ||
); | ||
if (!await fs.exists(corePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!await fs.exists(corePath)) { | |
if (!fs.existsSync(corePath)) { |
We need to use fs.existsSync
here because fs.exists
is a callback-style function (rather than promise-style) and deprecated.
src/utils/build.js
Outdated
return {}; // TODO err, what should we do on windows? | ||
const fileWriteError = await writeFile( | ||
`${tmpDir}/definition.json`, | ||
prettyJSONstringify(rawDefinition.results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to write
prettyJSONstringify(rawDefinition.results) | |
prettyJSONstringify(rawDefinition) |
because .results
has been done previously.
'zapier-' + crypto.randomBytes(4).toString('hex') | ||
); | ||
|
||
const maybeNotifyAboutOutdated = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for making it a function!
wdir = wdir || process.cwd(); | ||
zipPath = zipPath || constants.BUILD_PATH; | ||
sourceZipPath = sourceZipPath || constants.SOURCE_PATH; | ||
const osTmpDir = await fse.realpath(os.tmpdir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const osTmpDir = await fse.realpath(os.tmpdir()); | |
const osTmpDir = fse.realpathSync(os.tmpdir()); |
This needs to be sync version as fs.realpath
is a callback-style function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 TIL!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! If we had been using fs
, you'd be correct, but the whole point of fs-extra
is that all the async methods are promise-baed.
I think we're good on the |
I originally started this as an improvement to
source.zip
generation (namely, always excluding node_modules) but got distracted and inspired by your PR yesterday that I wanted to do the same. I'll get to that another day.No functionality should have changed - this is purely a rewrite using async and await.