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

Add lerna support and clean up all projects #112

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

maliroteh-sf
Copy link
Contributor

No description provided.

@maliroteh-sf maliroteh-sf requested a review from a team as a code owner December 12, 2023 13:58
@dbreese
Copy link

dbreese commented Dec 12, 2023

Huge PR, where should we focus? ;)

@maliroteh-sf
Copy link
Contributor Author

@dbreese Unfortunately due to moving project files around and fixing lots of prettier & linting errors, it's impossible to keep this PR small. I guess to verify, just clone locally and check that yarn install && yarn precommit will run/pass the linting & prettier & tests.

For me everything passes locally but in CI for some reason 1 test is failing so I still need to look into that.

@khawkins
Copy link
Collaborator

I see that only a subset of projects are configured to run tests. I'm presuming that's either because the others didn't have test targets, or their test targets don't work?

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

This looks fine to me in the broad details. In the future I'd recommend breaking this up into multiple PRs, to make it easier to review. Even though you're working on a lot of files, there are still ways to minimize the impact of reviewing those changes by breaking them up. For example:

  • First PR simply moves all projects under the projects/ umbrella. No further changes.
  • Second PR reformats all files with prettier. No further changes.
  • (Optional) 3rd to n PRs do the same for any other applicable targets (lint, test, etc.).
  • Final PR implements Lerna configuration changes to support the monorepo.

There were a number of changes aggregated in this PR, that would have been more easily reviewed by splitting them out.

Thanks for taking this on! It'll help manage all of the disparate projects a lot more consistently here. 👍

@maliroteh-sf
Copy link
Contributor Author

I see that only a subset of projects are configured to run tests. I'm presuming that's either because the others didn't have test targets, or their test targets don't work?

Yes some projects do not have tests. We can have follow up PRs to add tests to these sample LWC projects if we want to.

@maliroteh-sf maliroteh-sf merged commit f95fe31 into forcedotcom:main Dec 14, 2023
6 checks passed
@maliroteh-sf maliroteh-sf deleted the lerna branch December 14, 2023 20:46
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.

3 participants