Add a better error message when uploading with a zip #408
Conversation
src/utils/api.js
Outdated
|
|
||
| if (isMissingZip) { | ||
| return Promise.reject( | ||
| new Error('Missing a built app. Trying running `zapier push`.') |
There was a problem hiding this comment.
Alternatively, we could just console.error or catch this one level up and context.line. I'm in favor of more general error handling in entry where this ends up.
There was a problem hiding this comment.
I think I like that a little more, but it's not a blocker for me
There was a problem hiding this comment.
You're more in favor of console.error right here?
stevelikesmusic
force-pushed
the
fix/better-upload-messaging
branch
from
aebf3b8 to
d68f3bd
Compare
src/utils/api.js
Outdated
|
|
||
| if (isMissingZip) { | ||
| return Promise.reject( | ||
| new Error('Missing a built app. Trying running `zapier push`.') |
There was a problem hiding this comment.
I think I like that a little more, but it's not a blocker for me
src/commands/upload.js
Outdated
| } complete! Try \`zapier versions\` now!` | ||
| ); | ||
| }); | ||
| return utils |
There was a problem hiding this comment.
while you're in here, do you mind converting this to async? we've been doing that with other commands as we go, which will eventually remove our dependence on babel.
There was a problem hiding this comment.
I can definitely do that! Curious though—what's preventing us from dropping babel now? If it's just waiting to convert to async/await, I would think we could run the CLI in node natively.
stevelikesmusic
force-pushed
the
fix/better-upload-messaging
branch
from
4561d22 to
15b9a1a
Compare
| @@ -123,7 +123,7 @@ module.exports = argv => { | |||
| process.exit(1); | |||
| } | |||
|
|
|||
| commandFunc.apply(commands, [context].concat(args)).catch(err => { | |||
| commandFunc(context, ...args).catch(err => { | |||
There was a problem hiding this comment.
Left this .catch so we didn't grab any unintended Errors here.
There was a problem hiding this comment.
i was worried about this change but it looks like it didn't need to be passing the command object before, so we're good!
There was a problem hiding this comment.
Exactly, I checked and we're not even referencing this anywhere.
src/utils/api.js
Outdated
| const isMissingZip = !fs.existsSync(fullZipPath); | ||
|
|
||
| if (isMissingZip) { | ||
| throw new Error('Missing a built app. Trying running `zapier push`.'); |
There was a problem hiding this comment.
Thinking about it, this is a little bit of an odd error message. I'd probably change it to "Try running zapier build", since push will have automatically run build and if they're seeing this message, it's because they're manually calling upload before build.
Also there's a typo: Trying running
There was a problem hiding this comment.
Typos...
My thought is if they're running upload without build, there's a gap with their CLI knowledge. What if we at least say something like You need to run zapier build first. Or you could run zapier push, which will build and upload in one command.
| @@ -123,7 +123,7 @@ module.exports = argv => { | |||
| process.exit(1); | |||
| } | |||
|
|
|||
| commandFunc.apply(commands, [context].concat(args)).catch(err => { | |||
| commandFunc(context, ...args).catch(err => { | |||
There was a problem hiding this comment.
i was worried about this change but it looks like it didn't need to be passing the command object before, so we're good!
stevelikesmusic
force-pushed
the
fix/better-upload-messaging
branch
from
15b9a1a to
716b112
Compare
Description
When a dev attempts to
zapier uploadwithout having built the integration, we silently fail when trying to read the zip.Instead, we'll check to see if the zip exists, and then suggest running
zapier pushif we still need to build.Issue
#351