Server-side zapier convert
#380
Conversation
eliangcs
changed the title
zapier convertzapier convert
eliangcs
changed the title
zapier convertzapier convert
|
@FokkeZB this is something we talked about, so you might be interested in having a look, but please don't feel obligated to review. :) |
There was a problem hiding this comment.
Cool! Looks awesome.
I didn't read anything that was 100% removal, but I feel like I covered and understood everything else well enough. A couple of minor things, but no hard blockers if you feel like it's a non/issue has been resolved.
Thanks for grabbing this stuff!
| @@ -17,24 +17,27 @@ const convert = (context, appid, location) => { | |||
| return Promise.reject(new Error(message)); | |||
| } | |||
|
|
|||
| const createApp = tempAppDir => { | |||
| const url = `${ | |||
| const createApp = async tempAppDir => { | |||
There was a problem hiding this comment.
+1 to eventually getting all of cli to async // await!
src/utils/command-invocations.js
Outdated
| context.line( | ||
| "Looks like this is your first push. Let's register your app on Zapier." | ||
| ); | ||
| return getInput('Enter app title (Ctrl-C to cancel):\n\n ').then(title => | ||
| register(context, title, { printWhenDone: false }) | ||
| return await getInput('Enter app title (Ctrl-C to cancel):\n\n ').then( |
There was a problem hiding this comment.
minor, seems weird to return the promise chain here. Why not
const input = await getInput('Enter' // ...
await register(context, // ...
return nullAlso, i'm not sure we need the return statement at all.
There was a problem hiding this comment.
I totally missed it! Cleaned up in 4830272.
| templateContext, | ||
| prettify = true | ||
| ) => { | ||
| const templateBuf = await readFile(templateFile); |
There was a problem hiding this comment.
I believe this can be const template = await readFile(templateFile, 'utf-8'), since passing encoding gives you a string instead of a buffer.
There was a problem hiding this comment.
This readFile is a function we wrote in src/utils/files.js and doesn't accept an encoding option. It is used in other places, so I rather not change its interface for now.
src/utils/convert.js
Outdated
| const renderIndex = async appDefinition => { | ||
| let exportBlock = _.cloneDeep(appDefinition), | ||
| functionBlock = '', | ||
| importBlock = ''; |
There was a problem hiding this comment.
I'd probably do importBlock as an array and then .join('\n') at the end instead of needing to use a newline whenever you add to it.
There was a problem hiding this comment.
Good call! Changed in 3447bd7.
| includeInBuild: ['scripting.js'] | ||
| }); | ||
| await createFile(content, '.zapierapprc', newAppDir); | ||
| startSpinner('Writing .zapierapprc'); |
There was a problem hiding this comment.
presumably we'd want to start the spinner before beginning the async action?
There was a problem hiding this comment.
We're only using it to print messages and not really taking advantage of spinner here. Each output file is generated concurrently with Promise.all. To properly use spinner to show progress, we need to create a spinner instance for each promise, whereas we only have a global spinner instance in src/utils/display.js. Since conversion is very fast, spinner doesn't help that much, I wouldn't bother to create multiple spinner instances just to properly show progress.
| const destFile = path.join(dir, filename); | ||
| await ensureDir(path.dirname(destFile)); | ||
| await writeFile(destFile, content); | ||
| startSpinner(`Writing ${filename}`); |
There was a problem hiding this comment.
See my previous comment.
| silent = false | ||
| ) => { | ||
| if (silent) { | ||
| startSpinner = endSpinner = () => null; |
There was a problem hiding this comment.
I did some googling and i don't think this has any chance of messing up the import for other files (it's just redefining a local variable).
That said, I think i'd rather build this functionality into the spinner itself rather than do this each place we may need to maybe not have the spinner actually go. Maybe in utils, instead of exporting the object itself we export a wrapper class?
// wrapper pseudocode
class SpinnerWrapperClass {
constructor(silent=false)
startSpinner() {
if (this.silent) {return}
spinner.spin()
}
// usage
const {spinner} = require('./utils/display')
const s = new Spinner(silent)
s.startSpinner()slightly more verbose, but then we keep all functionality in the same spot. Not a blocker, just a thought.
There was a problem hiding this comment.
Currently, the silent option is there only for this test to avoid printing stuff in the console while running tests. It's true that the spinner utils could use some improvement but it's probably not worth doing only for a test.
Reworks the
zapier convertcommand so that it uses the up-to-date converter implementation on the backend. Requires https://github.com/zapier/zapier/pull/22060.This PR is quite big and can be unstraightforward to review, but the big ideas are:
cli-dumpendpoint (see https://github.com/zapier/zapier/pull/22060) that dumps a legacy WB app to a JSON app definition compatible with the CLI platform.zapier convertpulls the app definition from thecli-dumpendpoint.zapier convertsplits the app definition, a big JSON blob, into separate js files.