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

ci: use graph-type all for lerna publish #1084

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Nov 2, 2022

CD is broken since #1082 has been merged, and it seems that it's because lerna is using alphabetical order when publishing packages (lerna publish command), so attempts to build action-menu before core (marked as peerDependency).

According to Lerna Publish documentation, this is because lerna uses by default a dependencies graph-type, which will take into account only packages listed in dependencies to determine the build order. As in this case it is listed in peerDependencies, an all graph-type must be used.

Another option of course would be to modify action-menu's package.json to set core as a direct dependency. However, this would be also needed for any other package that comes before to core in alphabetical order (such as bbs-signatures). I'm not sure if this would be conceptually correct (this was discussed recently when extracting BBS module).

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris genaris requested a review from a team as a code owner November 2, 2022 11:05
@TimoGlastra
Copy link
Contributor

I see no harm in using dependencies over peer dependencies. For this repo all versions must be the same, so there's not really an advantage in using peer deps over deps. If we in the future start using separate versions for packages in this repo peer deps make sense. I'm impartial on this discussion.

If we go with the approach in this PR we should also update it for lerna publish for stable release (maybe even add it to lerna.json so we can't publish without)

@genaris
Copy link
Contributor Author

genaris commented Nov 2, 2022

If we go with the approach in this PR we should also update it for lerna publish for stable release (maybe even add it to lerna.json so we can't publish without)

I think adding "graphType": "all" in lerna.json is the best way to go instead of the --graph-type all I've added to the script.This way, all lerna release commands in the script will take this into account, right?

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris genaris enabled auto-merge (squash) November 2, 2022 15:27
@genaris genaris merged commit bd01e40 into openwallet-foundation:main Nov 2, 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.

2 participants