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

BREAKING CHANGE(cli): new zapier init with up-to-date templates #206

Merged
merged 35 commits into from
May 13, 2020

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Apr 29, 2020

Rewrites zapier init and updates all the project templates.

The new zapier init command is powered by Yeoman, so it has the interactive goodies like the Yeoman CLI yo.

This is a breaking change because:

  • --template no longer defaults to minimal. If a user doesn't specify --template, zapier init will prompt the user to choose a template.
  • Some templates aren't available anymore. These are what we currently have. And these are the ones we're going to keep:
    • basic-auth
    • custom-auth
    • digest-auth
    • oauth1-trello
    • oauth2
    • session-auth
    • dynamic-dropdown
    • files
    • minimal
    • search-or-create
    • typescript

@eliangcs eliangcs changed the title improvement(cli): init with yeoman improvement(cli): zapier yo, init with yeoman Apr 29, 2020
}

async prompting() {
if (!this.options.authType) {
Copy link
Member Author

@eliangcs eliangcs Apr 29, 2020

Choose a reason for hiding this comment

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

So this means we only ask for the auth type interactively when the user didn't provide --auth in the CLI.

class ProjectGenerator extends Generator {
initializing() {
this.sourceRoot(path.resolve(__dirname, 'templates'));
this.destinationRoot(path.resolve(this.options.path));
Copy link
Member Author

@eliangcs eliangcs Apr 29, 2020

Choose a reason for hiding this comment

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

These calls are parts of the Yeomen Generator API: https://yeoman.io/authoring/file-system.html

  • sourceRoot is where we keep the templates. We set it to packages/cli/generators/templates.
  • destinationRoot is the directory of the new project.

}
];

class ProjectGenerator extends Generator {
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice this class ProjectGenerator is in the generators directory. I use plural "generators" on purpose because I think we might want to add more sub-generators to power the scaffold command. For example, we could have a TriggerGenerator that powers zapier scaffold trigger foo.

this.sourceRoot(path.resolve(__dirname, 'templates'));
this.destinationRoot(path.resolve(this.options.path));

this.registerTransformStream(prettier({ singleQuote: true }));
Copy link
Member Author

@eliangcs eliangcs Apr 29, 2020

Choose a reason for hiding this comment

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

This is where we register prettier to format .js and .json files.

@eliangcs eliangcs requested a review from xavdid April 29, 2020 07:46
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

nice, this looks great!

Do we think we'll want to have other types besides auth? Or will those be the only template options? I think typescript will still be fair game.

@@ -48,6 +48,7 @@
"colors": "1.3.3",
"debug": "4.1.1",
"fs-extra": "8.1.0",
"gulp-prettier": "3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this included? the regular prettier has a format we should be able to use.

Copy link
Member Author

@eliangcs eliangcs Apr 30, 2020

Choose a reason for hiding this comment

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

Good question! It's because Yeoman generator's registerTransformStream requires a vinyl object stream (like gulp) rather than a simple function: https://yeoman.io/authoring/file-system.html#transform-output-files-through-streams

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, makes sense.

maybe a dumb question, but it doesn't look like we'll actually be feeding prettier in the generator any un-pretty files. the generated auth stuff is run through prettier, and presumably any templates we ship will have also been prettyified already. so i'm not sure this will actually do anything here.

Copy link
Member Author

@eliangcs eliangcs Apr 30, 2020

Choose a reason for hiding this comment

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

We'll still need to run prettier for non-auth files, such as package.json and index.js. Since here this Yeoman generator runs prettier before writing any files, maybe we don't need to do any code formatting when generating auth stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

this comment was previously misplaced from an email, so i'm reposting it


That's true. It looks like the index file is already pretty as it's stored here and package.json is easy to pretty print, but it's nbd either way. An extra dep doesn't hurt much.

},
devDependencies: {
mocha: '^5.2.0',
should: '^13.2.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think i'm going to try to swap at least should in the new templates - it's also been a long-held frustration with me. I find it very backwards.

jest has expect included so we could use that as a runner. I've been super impressed with the guard rails and suggestions they provide (for instance, suggesting to use different or better assertion functions when you'll get unexpected behavior). I think we could also use that separate though if you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not a fan of should either. I'll give jest a test today.

@eliangcs
Copy link
Member Author

Do we think we'll want to have other types besides auth? Or will those be the only template options? I think typescript will still be fair game.

The CLI will get complicated if it has both --auth and --template options. If we do that, I imagine we'll have to deal with num_of_auth_options * num_of_template_options combinations. For example, we'll need to prepare a typescript template for all six auth types.

A more practical way is to provide either --auth or --template, but not both. So if we only provide --template, what templates you think we should offer? So if we have:

  • basic
  • custom
  • digest
  • oauth1
  • oauth2
  • session
  • typescript

The last one typescript looks awkward to me because it's not an auth type like the rest.

I don't have a good solution yet, but I'm wondering do you any thoughts? @xavdid

@xavdid
Copy link
Contributor

xavdid commented Apr 30, 2020

this comment was previously misplaced from an email, so i'm reposting it


Yeah, that's fair. I would think typescript would just be a TS version of a single other template (maybe the basic one) but maybe we drop it for now and do it in docs instead of a full template. If we wanted to include it, the auth and typescript flags could be mutually exclusive. You can get either the specific auth or a basic typescript template? I could go either way.

Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

nice! This template work can be so tedious.

Left a couple of minor things that aren't blockers.

We're at the end of the week, so depending on how much else we've got going on, we should estimate remaining workload and timeframe to get this out the door. We can touch base on that next week!

},
engines: {
node: `>=${NODE_VERSION}`,
npm: '>=5.6.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure we need this anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need this either. I'll remove the entire engines property.

test: 'jest'
},
engines: {
node: `>=${NODE_VERSION}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly probably don't need this either - it mostly causes confusion. We enforce the node version w/ the CLI and their core version sets it for runtime. Plus it's not even enforced.

};

const writeGenericAuthTest = gen => {
gen.fs.write(path.join('test', 'authentication.js'), '// TODO\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

we're going to copy these from the examples directory; there's not enough work to bother templating them.

};

const writeGenericAuth = gen => {
gen.fs.write('authentication.js', '// TODO\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

once you import the new utils/auth-files-codegen, this will be authFileFuncs[authType](). They don't go through prettier on that end

@eliangcs eliangcs changed the title improvement(cli): zapier yo, init with yeoman improve(cli): new zapier init with up-to-date templates May 13, 2020
@eliangcs eliangcs changed the title improve(cli): new zapier init with up-to-date templates BREAKING CHANGE(cli): new zapier init with up-to-date templates May 13, 2020
@eliangcs eliangcs marked this pull request as ready for review May 13, 2020 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants