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

feat: Add support for remote templates #261

Merged
merged 10 commits into from Mar 27, 2020

Conversation

fmvilas
Copy link
Member

@fmvilas fmvilas commented Mar 20, 2020

Description

Changes this PR introduces:

  1. All templates are now treated as "remote".
  2. Renames the -w, --watch flag to --watch-template. Let's leave -w, --watch for "watching" the AsyncAPI file. Shows a warning when the template directory under node_modules is not a symlink (i.e., it's a remote template).
  3. Slightly changes the meaning and name of --force-install. Its name is -i, --install now. It will install the template and its dependencies.
  4. Removes the -t, --templates <templateDir> flag. Keep reading to know why.
  5. Removes the "templates" directory 😱. Don't panic, they have been extracted to their own repo: https://github.com/search?q=topic%3Aasyncapi+topic%3Agenerator+topic%3Atemplate

This is how it feels now on the CLI:

# Use template from npm (skip installation if already installed)
ag asyncapi.yaml @asyncapi/html-template -o output

# Use template from npm (forces installation)
ag asyncapi.yaml @asyncapi/html-template -i -o output
ag asyncapi.yaml @asyncapi/html-template -io output
ag asyncapi.yaml @asyncapi/html-template -o output --install

# Use template from npm (watches template for changes)
ag asyncapi.yaml @asyncapi/html-template -o output --watch-template

# Use template from filesystem (skip installation if already installed)
ag asyncapi.yaml ../my-templates/asciidoc -o output

# Use template from filesystem (forces installation)
ag asyncapi.yaml ../my-templates/asciidoc -io output
ag asyncapi.yaml ../my-templates/asciidoc -o output --install

# Use template from filesystem (watches template for changes)
ag asyncapi.yaml ../my-templates/asciidoc -o output --watch-template

Related issue(s)
Fixes #251

To do

  • Update docs

@fmvilas fmvilas self-assigned this Mar 20, 2020
@fmvilas fmvilas changed the title Add support for remote templates 🎉 feat: Add support for remote templates Mar 20, 2020
cli.js Outdated Show resolved Hide resolved
lib/generator.js Outdated Show resolved Hide resolved
@fmvilas fmvilas marked this pull request as ready for review March 20, 2020 18:43
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I had some issues trying to generate with our templates, check out my comments.
General comments:

  • if we do not want to have fore-install flag, should we still refer to it in the code as force installation? like * @param {Boolean} [options.forceInstall=false] Force the installation of the template and its dependencies. It is a bit confusing
  • readme needs some love. There is no information about remote and local templates. There is no info about the fact that we support npm/git/local/tar. In related GitHub issue you have it well explained/listed. I think we need to:

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@fmvilas fmvilas requested a review from derberg March 24, 2020 17:48
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I addded just few comments, and also what about the my comment to forceInstall?

when I checked with git and tar, the installation is always triggered, even if I already used the template once:

wookiee$ ./cli.js ./test/docs/streetlights.yml git://github.com/asyncapi/html-template.git -o ./output  --force-write
npm http fetch GET 304 https://registry.npmjs.org/markdown-it 712ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/openapi-sampler 796ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/entities 167ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/json-pointer 165ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/foreach 151ms (from cache)
+ @asyncapi/html-template@0.2.0
updated 1 package in 7.884s

21 packages are looking for funding
  run `npm fund` for details



Done! ✨
Check out your shiny new generated files at /Users/wookiee/Documents/sources/generator-add-support-for-remote-templates/output.

wookiee$ ./cli.js ./test/docs/streetlights.yml git://github.com/asyncapi/html-template.git -o ./output  --force-write
npm http fetch GET 304 https://registry.npmjs.org/openapi-sampler 622ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/markdown-it 648ms (from cache)
npm http fetch GET 200 https://registry.npmjs.org/entities 5ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/json-pointer 157ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/foreach 169ms (from cache)
+ @asyncapi/html-template@0.2.0
updated 1 package in 7.24s

21 packages are looking for funding
  run `npm fund` for details



Done! ✨
Check out your shiny new generated files at /Users/wookiee/Documents/sources/generator-add-support-for-remote-templates/output.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/generator.js Outdated Show resolved Hide resolved
lib/generator.js Outdated Show resolved Hide resolved
lib/generator.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fmvilas
Copy link
Member Author

fmvilas commented Mar 26, 2020

@derberg all comments have been addressed now.

when I checked with git and tar, the installation is always triggered, even if I already used the template once:

In order to know the name of the package —and know if it has been installed already— npm has to first download it to look into the package.json file. That's why it's always downloaded :)

@fmvilas fmvilas requested a review from derberg March 26, 2020 20:01
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Checked functionality with all possible cases:

  • tar works like a charm ./cli.js ./test/docs/streetlights.yml https://github.com/asyncapi/html-template/archive/v0.2.0.tar.gz -o ./output --force-write
  • npm works like a charm ./cli.js ./test/docs/streetlights.yml @asyncapi/html-template -o ./output -p title='Hello from param' --force-write
  • github works like a charm ./cli.js ./test/docs/streetlights.yml https://github.com/asyncapi/nodejs-template -o ./output -p server=production --force-write
  • local works like a charm ./cli.js ./test/docs/streetlights.yml ../nodejs-template -o ./output -p server=production --force-write
  • npm with a specific version works like a charm ./cli.js ./test/docs/streetlights.yml @asyncapi/html-template@0.1.0 -o ./output -p title='Hello from param' --force-write
  • github with a specific tag works like a charm ./cli.js ./test/docs/streetlights.yml git://github.com/asyncapi/html-template.git#v0.1.0 -o ./output --force-write

Works like a charm.
This feature is a killer. Well done! 🚀

@fmvilas fmvilas merged commit 543f9d5 into asyncapi:master Mar 27, 2020
@fmvilas fmvilas deleted the add-support-for-remote-templates branch March 27, 2020 12:20
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.35.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make generator support installing remote templates
4 participants