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

fix: ACNA-1687 | fix option skipInstall for generators #561

Conversation

arjuncooliitr
Copy link
Contributor

@arjuncooliitr arjuncooliitr commented Jul 25, 2022

Fixed option skipInstall for yeoman generators used for scaffolding App Builder Apps

Description

This PR aims to fix skipInstall options issue with yeoman generators. There is a slight change to how to use this option to make it work.

Related Issue

#554

Motivation and Context

npm dependencies seem to be installed twice once after the base-app gets generated and at the end before the app initialization is over
Expected behaviour: npm dependencies should only be installed at the end of the project initialization once the package.json deps are all defined.

How Has This Been Tested?

Locally tested by creating an app - npm packages are now installed only at the end. Generators skip installing packages. All units tests run successful locally.

Screenshots (if appropriate):

Generators skipping installing packages:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #561 (472e77c) into master (ac0cd12) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #561   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           50        50           
  Lines         2598      2607    +9     
  Branches       473       473           
=========================================
+ Hits          2598      2607    +9     
Impacted Files Coverage Δ
src/commands/app/add/action.js 100.00% <100.00%> (ø)
src/commands/app/add/ci.js 100.00% <100.00%> (ø)
src/commands/app/add/event.js 100.00% <100.00%> (ø)
src/commands/app/add/extension.js 100.00% <100.00%> (ø)
src/commands/app/add/web-assets.js 100.00% <100.00%> (ø)
src/commands/app/delete/ci.js 100.00% <100.00%> (ø)
src/commands/app/init.js 100.00% <100.00%> (ø)
src/lib/vscode.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@arjuncooliitr arjuncooliitr marked this pull request as ready for review July 26, 2022 11:15
@arjuncooliitr arjuncooliitr changed the title fix: ACNA-1687 | fixed option skipInstall for appGen, extGen, addActi… fix: ACNA-1687 | fixed option skipInstall for generators Jul 27, 2022
@arjuncooliitr arjuncooliitr changed the title fix: ACNA-1687 | fixed option skipInstall for generators fix: ACNA-1687 | fix option skipInstall for generators Jul 27, 2022
shazron added a commit to adobe/aio-cli-plugin-app-templates that referenced this pull request Jul 28, 2022
…n (install command) (#19)

* fix: when the template is run, the options were not being passed to the generator

The previous usage of env.run, the options are actually the run options, and not the generator options. This has the proper usage.

Reference: https://yeoman.github.io/environment/Environment.html#run

* fix: ACNA-1732 - yeoman run skip-install work-around

See adobe/aio-cli-plugin-app#561

* fixed README.md with oclif readme markers, then re-gen docs

docs are generated with `npm run prepack` (which is run before the package is published usually)
Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

LGTM, looks like you had to do the same change more times than I can count 😄.

@purplecabbage
Copy link
Member

This looks good.
Please remove the repeated commented out line:

// Moving ['skip-install': true] to env.options due to yeoman environment issue https://github.com/yeoman/environment/issues/421

We can use git blame or other means to go back in time ... no need to maintain historical code in the comments.

Copy link
Member

@purplecabbage purplecabbage left a comment

Choose a reason for hiding this comment

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

Just the comment about removing the commented code ... looks good!

@arjuncooliitr
Copy link
Contributor Author

Hi @purplecabbage, Thanks for reviewing. I have removed the historic code comment. Can I merge this PR now?

@arjuncooliitr arjuncooliitr merged commit 163d3a3 into adobe:master Aug 16, 2022
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.

4 participants