Convert visual apps to CLI. #427

xavdid · 2019-05-08T22:08:43Z

Standing on the shoulders of other great work, this ended up being pretty straightforward!

I liked the idea of re-using the command. That, plus adding the version option, makes it pretty straightforward.

There are a couple of different methods based on differences between what we need for visual/legacy, but the similarity made that part really easy.

I've been testing with the following:

zapier convert 17741 a --version=1.0.1, which is a dev-app-school wistia app. I've confirmed that you can convert, push a new version, and make a new zap (auth + trigger) successfully.

stevelikesmusic · 2019-05-09T19:21:57Z

src/commands/convert.js

@@ -2,7 +2,7 @@ const utils = require('../utils');
 const constants = require('../constants');

 const convert = (context, appid, location) => {
-  context.line('Welcome to the Zapier Platform! :-D');
+  context.line('Welcome to the Zapier Platform!');


Do we even need this message at all?

no, not really!

stevelikesmusic · 2019-05-09T20:10:36Z

package.json

    "lint-snippets": "eslint snippets --rule 'no-unused-vars: 0' --ignore-pattern snippets/next.js --ignore-pattern 'snippets/dynamic-dropdowns-*'",
+    "lint": "eslint zapier.js src",
+    "postlint": "npm run lint-snippets",


stevelikesmusic

Nice work @xavdid! I like the leveraging of the current convert tool—great decision. We will still need to tweak its internals a bit. Especially for getting dynamicInputs which are in a broken state as is. I'm marking as Request Changes for that, but the rest is suggestions, questions, and such.

A few thoughts:

Would we want devs to run without passing a version? I would expect most would want to use the latest version to convert.
Do we want to order the fields in index.js? It'd be nice if they were properly sorted like the minimal example app.
What about instead of using conditionals to fork logic, what if we had handlers for each source? (legacyHandler, vbHandler). Might be a little easier to follow, although we only have two choices right now. Other app sources could use an import command.

Still wrapping my head around what the rest of convert is doing and where we'd need to add some new logic.

Love the tests! 💪

stevelikesmusic · 2019-05-09T20:17:45Z

src/commands/convert.js

+        }
+      }
+      utils.endSpinner();
+      return utils.convertVisualApp(appInfo, versionInfo, tempAppDir);


Since we're going to throw in the catch why not move these two lines into the try. Then we won't need to set empty vars.

try { [appInfo, versionInfo] = await Promise.all([ utils.callAPI(null, { url: appInfoUrl }, true), utils.callAPI(null, { url: versionInfoUrl }, true) ]); utils.endSpinner(); return utils.convertVisualApp(appInfo, versionInfo, tempAppDir); } catch (e) { throw ...

stevelikesmusic · 2019-05-10T10:21:38Z

src/utils/convert.js

+const convertApp = async (appInfo, appDefinition, newAppDir, opts = {}) => {
+  const defaultOpts = { silent: false, legacy: true };
+  // need to bump babel so this works
+  // const { silent, legacy } = { ...defaultOpts, ...opts };


The spread operator works in node v8. We can either lose these comments or switch to that syntax.

for sure. Our current version of babel throws an error here, so I think i'll:

merge try removing babel #430

merge develop back into this PR,

uncomment this

i'll do that after we're 👍 though to keep this PR cleaner. I think the babel one will make it loud

stevelikesmusic · 2019-05-10T10:22:24Z

src/utils/convert.js

+};
+
+const convertApp = async (appInfo, appDefinition, newAppDir, opts = {}) => {
+  const defaultOpts = { silent: false, legacy: true };


Is there any reason to even have silent? I may be missing where it's actually used.

the only reason is the tests.

with silent:

$ NODE_ENV=test mocha -t 50000 --recursive lib/tests convert visual apps apps ✓ should create separate files (110ms) 1 passing (125ms) ✨ Done in 3.65s.

without silent:

$ NODE_ENV=test mocha -t 50000 --recursive lib/tests convert visual apps apps ✔ Writing .gitignore ✔ Writing triggers/project.js ✔ Writing package.json ✔ Writing creates/create_project.js ✔ Writing .env ✔ Writing authentication.js ✔ Writing .zapierapprc ✔ Writing .zapierapprc ✔ Writing triggers/codemode.js ✔ Writing index.js ✔ Writing test/creates/create_project.js ✔ Writing test/triggers/project.js ✔ Writing test/triggers/codemode.js ✓ should create separate files (115ms) 1 passing (132ms) ✨ Done in 3.66s.

Could get a similar effect by only logging if NODE_ENV !== 'TEST' if we wanted to simplify it a little bit

Thanks @xavdid! That makes sense.

NODE_ENV could be a good option. No strong feelings one way or the other.

stevelikesmusic · 2019-05-10T10:27:17Z

src/tests/utils/convert.js

+    });
+  });
+
+  describe.only('visual apps apps', () => {


I'm going to add eslint-jest to prevent these from making it into PRs. I've snuck those into PRs a few times and didn't catch until later.

stevelikesmusic · 2019-05-10T10:28:29Z

src/tests/utils/convert.js

+            params: {},
+            method: 'POST'
+          },
+          inputFields: [


🚨 We should add a dynamic input field for testing. These are currently getting written as a giant string:

~~can you clarify what you mean by dynamic input field? I know in CLI we can have custom fields, but I didn't think that was something that was possible in godzilla~~

was only looking in triggers, not actions. found it and fixed it

stevelikesmusic · 2019-05-10T10:40:41Z

src/commands/convert.js

-      utils.callAPI(null, { url: cliDumpUrl })
-    ]);
+      utils.startSpinner('Downloading app from Zapier');
+      let [legacyApp, appDefinition] = [];


Same here as the vb-way above.

stevelikesmusic · 2019-05-10T10:47:47Z

src/tests/utils/convert.js

+      create_project: {
+        operation: {
+          perform: {
+            body: {


Do we want to keep shorthand requests in a CLI integration? Do we care about encouraging using curlies within CLI? Not a big deal now that we support it because of Godzilla. Just wanted to point out that we'll be encouraging devs to rely on curly string replacement rather in core than regular ol' JS.

I think we do want to keep the shorthand requests. They provide some nice auto error-handling and other sugar that are perfectly reasonable to rely on if you don't need a more complex request. Curlies should also be fine, though #428 makes me a little worried about that.

stevelikesmusic · 2019-05-10T10:59:57Z

src/utils/convert.js


  return await Promise.all(promises);
 };

+const convertVisualApp = async (


What would you say to re-organizing this slightly? This assumes we drop silent, but we could still keep it if we really wanted. We could pass in definition_override rather than versionInfo and use partial application to simplify these to functions:

const makeConvertApp = options => (info, def, dir) => ..stuff const convertVisual = makeConvertApp({ legacy: false }); const convertLegacy = makeConvertApp({ legacy: true });

or if we thought we might want to call convertApp directly in the future, we could still change the signature of convertApp and move options to the front

const convertApp = (options, info, def, dir) => ...stuff const convertVisual = convertApp.bind(null, { legacy: false }); // or with lodash const convertLegacy = _.partial(convertApp, { legacy: true });

Also, no need to wrap in async since we're not awaiting anything.

I'll do you one better - we can _.partialRight and not move any of the args!

And boom goes the dynamite 💥

stevelikesmusic · 2019-05-10T11:11:59Z

src/commands/convert.js


-    return await utils.convertApp(legacyApp, appDefinition, tempAppDir);
+      return utils.convertLegacyApp(legacyApp, appDefinition, tempAppDir);
+    }
  };

  return utils.initApp(context, location, createApp).then(() => {


Why not instead of asking for the project location, we use the same string that we put in name for the package.json? (kebab-cased app title) I think it'd be rare that a dev would want those names to be different.

mostly so that devs can run the command from anywhere. zapier convert 1234 ../../projects. Since we're writing (and potentially overwriting) files, seems good to be explicit about it.

I hear you. I wonder if that's what most do?

It'd be nice to have the option to just run zapier convert, get a list of integrations, pick the integration I want, and confirm things like version and/or location. Not for this PR—just thinking out loud.

totally. I think this is good to get it out and we can revisit it at some point.

stevelikesmusic · 2019-05-10T11:30:57Z

src/tests/utils/convert.js

+  });
+
+  describe.only('visual apps apps', () => {
+    it('should create separate files', async () => {


I'm wondering if we need tests to verify the output of files 🤔 For instance, to ensure we get dynamic inputs, functions are correctly written, files are ordered correctly, etc.

Maybe overkill?

nah, more tests are good! I can cover a couple of key cases in there.

xavdid · 2019-05-21T23:04:01Z

Great questions!

I like being explicit about the version. It also reduces the number of API calls, since we only check for WB or godzilla. We could probably make both calls every time because it's unlikely that a dev has access to both CLI app 1234 and WB app 1234. We could also do a nice dropdown selector if we wanted, though that's probably out of scope here (we'd want to switch our prompt library from custom to off-the-shelf which is something I want to do anyway, but maybe not here)
ordering could be nice, I'll look at that!
I like that idea, but it sounds harder to maintain. As far as I can tell, you're proposing the following:

const convertLegacyApp(...) => {
  a()
  b()
  c()
  d()
}

const convertVisualApp(...) => {
  a()
  b()
  j()
  k()
  d()
}

where we move a lot of the logic into the reused functions. I like that, but worry that if we decide we want to add something after b, we now have to add that in multiple places (currently 2, but maybe more in the future). IMO, sounds tougher. But let me know if I'm misunderstanding!

xavdid · 2019-05-22T03:19:16Z

ok, addressed most of those. let me know what you feel strongly about and we can get this over the line!

stevelikesmusic

Thanks for the changes and tests @xavdid! I think this is looking good. Couple more questions, but nothing I'd call a blocker.

Re the handler approach, I was thinking more instead of:

if (isVb) {
  const preprocessed =  ...doSomeVbStuff
  convertVb(preprocessed);
} else {
  const preprocessed =  ...doSomeWbStuff
  convertLegacy(preprocessed);
}

we might do something like (total pseudo-code):

const convert = getConvertHandler(sourceType);
convert(appData);

The idea being, we'd encapsulate what would be a separate conditional block in each handler. Maybe a pre-mature optimization at this point.

stevelikesmusic · 2019-05-22T17:42:25Z

src/utils/convert.js

+          const args = maybeFunc.args || ['z', 'bundle'];
+          // always increment the number, but only return a value if it's > 0
+          const funcName = `get${_.upperFirst(key)}${
+            funcNum ? funcNum++ : ++funcNum && ''


Cool. So it would go as getInputFields, getInputField1, getInputFields2...

yep! read then increment.

> let i = 0 undefined > console.log(i++) 0 undefined > i 1 > i 1 > console.log(i++) 1 undefined > i 2 > console.log(++i) 3 undefined > i 3 > console.log(++i) 4 undefined > i 4

stevelikesmusic · 2019-05-22T17:44:22Z

src/commands/convert.js

@@ -2,11 +2,7 @@ const utils = require('../utils');
 const constants = require('../constants');

 const convert = (context, appid, location) => {
-  context.line('Welcome to the Zapier Platform! :-D');


It would be really cool here to clear out the location folder if it exists. I forgot to pass version to the tool, and it converted my vb app as if it was WB. When I ran it again, it just added to folder rather than replace it.

good call out. there's another place we check for an empty folder- we can borrow that behavior.

stevelikesmusic · 2019-05-22T17:55:50Z

Do we still have a separate ticket to build out the interface for this command? I thought there was also a ticket that would output a list of integration you had before executing the convert.

It's PDE-797

xavdid · 2019-05-22T18:16:04Z

We still have a separate ticket, but a lot of that work is in this one.

We can launch this as is ASAP, but for the next major we could change the UI to have a nice selection setup. So many plates in the air!

xavdid · 2019-05-23T01:34:18Z

Ah, so it turns out sorting keys is possible, but non-trivial if you have nested objects (which we do, in spades): https://stackoverflow.com/a/16168003/1825390

I'll skip for now but it'd be a nice cleanup thing to come back to (or render manually sometime). I'm going to go ahead and merge this as is.

xavdid added 4 commits May 8, 2019 00:49

plug into existing conversion code

9ef673c

add basic test

16a96b4

spruce command and polish tests

8526ba3

Fix hardcoded info in index.js

5087bca

xavdid requested a review from stevelikesmusic May 8, 2019 22:08

xavdid mentioned this pull request May 8, 2019

POC for scaffolding an app from a data source #425

Closed

stevelikesmusic reviewed May 9, 2019

View reviewed changes

stevelikesmusic suggested changes May 10, 2019

View reviewed changes

cleaned up convert

50f1d36

stevelikesmusic approved these changes May 22, 2019

View reviewed changes

xavdid added 4 commits May 22, 2019 21:39

tweak util function

0150d9e

merge back master

7027a41

better merge

173ce74

bump lockfile

7e25051

xavdid merged commit e9f6d05 into master May 23, 2019

xavdid deleted the convert-visual branch May 23, 2019 02:50

xavdid mentioned this pull request Jun 12, 2019

scaffold apps from the monorepo zapier/zapier-platform#4

Merged

Convert visual apps to CLI. #427

Convert visual apps to CLI. #427

Conversation

xavdid commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelikesmusic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid commented May 21, 2019

xavdid commented May 22, 2019

stevelikesmusic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelikesmusic commented May 22, 2019 • edited Loading

xavdid commented May 22, 2019

xavdid commented May 23, 2019

xavdid May 21, 2019 •

edited

Loading

stevelikesmusic commented May 22, 2019 •

edited

Loading