feat(icon): moved icon generation script

[tveinfeld]

• Moved icon generation script from /package.json -> /components/icon/package.json
• Added verbosity flag to generation script

[github-actions]

🚀

Latest successful build of the PR deployed here.

🚀

[yinonov]

Suggested change

	run: npx lerna run "generate-icon-resolver" --parallel --scope="@vonage/vwc-icon" -- --verboseOutput --resolveStrategy=cdn --awsBucketName=vivid-icons-prod --awsBaseUrl=https://icons.resources.vonage.com --awsAccessKey=${{ secrets.AWS_VIVID_DEMO_PROD_ACCESS }} --awsAccessSecret=${{ secrets.AWS_VIVID_DEMO_PROD_SECRET }}
	run: yarn lerna run "generate-icon-resolver" --parallel --scope="@vonage/vwc-icon" -- --verboseOutput --resolveStrategy=cdn --awsBucketName=vivid-icons-prod --awsBaseUrl=https://icons.resources.vonage.com --awsAccessKey=${{ secrets.AWS_VIVID_DEMO_PROD_ACCESS }} --awsAccessSecret=${{ secrets.AWS_VIVID_DEMO_PROD_SECRET }}

[tveinfeld]

I'm not sure it will yield the same result:
I think that yarn lerna will try to execute a package.json script named "lerna" (which does not exist), whereas npx lerna will run the default bin for package lerna.

[gullerya]

It won't, obviously.
I'm referring to make it back as it was: yarn generate-icon-resolver just as you have it in a build script.

[tveinfeld]

I was asked by @yinonov to remove this script from /package.json. In fact, this was the guiding motivation behind this PR.
As for myself: I don't hold a firm opinion as to where this script fits best. Therefore I suggest that @gullerya and @yinonov will discuss their opinions and reach a conclusion.

[gullerya]

What makes you think that our opinions are different? Don't push us where we aren't :)
The script can be run via yarn pointing to the package folder, as it was beforehand and there is no other opinion expressed on this specific point.

[github-actions]

File	Coverage
All files	80%	✅
common/context/src/vvd-context.ts	96%	✅
common/fonts/src/vvd-fonts.ts	83%	✅
common/foundation/src/form-association/associate-with-form.ts	89%	✅
common/foundation/src/form-association/common.ts	90%	✅
common/foundation/src/form-association/submit-on-enter-key.ts	80%	✅
common/scheme/src/os-sync.utils.ts	58%	❌
common/scheme/src/vvd-scheme-style-tag-handler.ts	79%	✅
common/scheme/src/vvd-scheme.ts	82%	✅
components/audio/src/vwc-audio.ts	60%	❌
components/button/src/vwc-button.ts	78%	✅
components/carousel/src/vwc-carousel.ts	71%	❌
components/drawer/src/vwc-drawer.ts	42%	❌
components/fab/src/vwc-fab.ts	63%	❌
components/file-picker/src/vwc-file-picker.ts	67%	❌
components/icon-button-toggle/src/vwc-icon-button-toggle.ts	67%	❌
components/icon-button/src/vwc-icon-button.ts	67%	❌
components/icon/src/vwc-icon.js	92%	✅
components/list/src/vwc-list-expansion-panel.ts	73%	❌
components/list/src/vwc-list-item.ts	80%	✅
components/media-controller/src/vwc-media-controller.ts	84%	✅
components/select/src/vwc-select.ts	90%	✅
components/slider/src/vwc-slider.ts	88%	✅
components/textarea/src/vwc-textarea.ts	81%	✅
components/textfield/src/vwc-textfield.ts	90%	✅
components/theme-switch/src/vwc-theme-switch.ts	88%	✅

Minimum allowed coverage is 75%

Generated by 🐒 cobertura-action against f83f106

[gullerya]

same as below, can you please use yarn to preserve convention as elsewhere?

[tveinfeld]

Are you sure this will yield the same result? (see my comment☝️)

[gullerya]

I mean yarn generate-icon-resolver, sorry, mislead you from what I've meant.

[gullerya]

@tveinfeld : beside the ask to replace the npx with yarn - why do you need that --parallel and lerna in general?

[tveinfeld]

@tveinfeld : beside the ask to replace the npx with yarn - why do you need that --parallel and lerna in general?

I use --parallel to make lerna output it's stdout as the command it runs executes (as opposed to waiting for it to complete, and then output the entire stdout buffer).

Using Lerna makes it easier to refactor folder structure (like the one @yinonov is promoting), since it can run scripts according to the npm package identifier rather than folder names.

[gullerya]

Then I'd say it is less than obvious and quite not clear here: lerna is used mainly to serve multi-package structure - not your case here; parallel - meant to run all the same scripts in all the packages - not your case here.
I think that until you'll really need that one - please use the straight forward syntax - which you actually had before and it worked just well.

[tveinfeld]

Then I'd say it is less than obvious and quite not clear here: lerna is used mainly to serve multi-package structure - not your case here; parallel - meant to run all the same scripts in all the packages - not your case here.
I think that until you'll really need that one - please use the straight forward syntax - which you actually had before and it worked just well.

Good catch! I've changed "parallel" to "stream" to enhance readability. As for the proper location for the script, well, I leave this decision to anyone who holds a firmer opinion than mine. @yinonov for one.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[gullerya]

@tveinfeld : i'm not talking about location of the script, where do you take it from?!
All I'm saying is that it is better to execute it by yarn and no point in all that lerna ceremony here, which adds more obscurity than any value.

[tveinfeld]

@gullerya:

In order to use yarn, i need to define a script in package.json, and this is exactly what @yinonov wishes to remove.

Now, if i want to execute the local script using yarn, i have to chdir into it and run the script, hence create coupling with the folder name in the build yml. to avoid this, lerna comes handy.

[gullerya]

Finally we getting closer :)
So @yinonov do NOT simply want to remove it, but move from global to local, which is not the same and I'm pretty sure it is clearly understandable from the beginning.
Yes, running locally is to do cd, which is exactly what you had previously and which is perfectly fine now as well.
Lerna happens to help here, but in general a wrong tool to use when not in context of multi-package stuff.

Let's discuss the whole thing tomorrow, since we probably see here symptoms of being the whole part of icons build mis-located, so it might be a good point to fix stuff.

[tveinfeld]

Lerna happens to help here, but in general a wrong tool to use when not in context of multi-package stuff.

we are using it in a context of multi package repo 😅

Lemme sum up the motivation. behind this PR for clarity:
yinon wanted to remove the icon creation specific script from /package.json and suggested using yarn package:build with a scope from the github yaml. Now, since the "build" script of icon contains two steps, and only one is relevant for github, i couldn't do it verbatim. instead, i've defined a local script and simply run it using lerna. This solution accommodates Yinon's request without impacting clarity in comparison to using yarn package:build.

Using yarn would force me to reintroduce a script to the root's package.json

i don't see how this exposes a deeper issue with the location of the icon module creation script, but it's a good opportunity to hear your opinion.

[gullerya]

Since it becomes too lengthy and not converging I'll ask a simple short one: why cd packages/icon && yarn generate-icon-script is not an option?

[tveinfeld]

it is definitely an option.. it's just that @yinonov offered a different approach of using Lerna with a scope instead.
I actually liked it because it reduces coupling from the folder structure (which proves beneficial right now with Yinon's folder refactoring suggestion).
It's not a big improvement, but it's nice to address packages by their official name rather than their monorepo folder name.
We could also just change the script on /package.json so that it uses Lerna with a scope, but that voids this PRs original intention.

[YonatanKra]

Pull Request Test Coverage Report for Build 453054845

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.5%) to 82.393%

---

Totals
Change from base Build 452981805:	-0.5%
Covered Lines:	848
Relevant Lines:	953

---

💛 - Coveralls

The proposal is not relevant since it requires a broader changes and looks like @yinonov has no meant it to be that way.