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

chore: update lerna to 5.1 and enable nx #5759

Merged
merged 14 commits into from
Jul 14, 2022
Merged

chore: update lerna to 5.1 and enable nx #5759

merged 14 commits into from
Jul 14, 2022

Conversation

vsavkin
Copy link
Contributor

@vsavkin vsavkin commented Jun 13, 2022

A few notes:

  • Just setting useNx to true should not break anything. You will just get a nicer terminal output. Turning on caching can break things cause you need to make sure operations are idempotents and the outputs are configured correctly (if the defaults do not work).
  • I enabled caching for test and build.
  • I removed --stream cause it looks much better without it. You can add it back if you want.
  • I configured it not to have topological sorting. Your build and tests don't depend on each other (from what I can tell :)).
  • From what I can see you don't generate any coverage reports when running tests, so the only thing that is cached is the terminal output.
  • I made some changes to the build.
    • build:js and build:types both write to the same dir, which means that the ownership of the folder is mixed. As far as I understand this is done for perf reasons, but since caching should make things perform well anyway (cause most of the time most builds should be cached), always invoking build is simpler.
      • If you strongly prefer having two targets, I could cache both build:js and build:types and configure outputs as globs.
    • You have the build:types script at the root, which I assume is to speed up the build process. This is done outside of Lerna so the results cannot be cached. I removed it. Again, the Lerna caching should make everything faster.
      • Nx is flexible. It allows you to write a custom executor that would split the results of running a single process (root tsc) into multiple tasks and cache them separately (as if they happened in separate processes) but imo it's not necessary in your case. It's usually used in massive enterprise repos.
  • I didn't enable Nx Cloud but you can (or I can do it for you). It'll be unlimited and free. At the very least you can get the remote caching and structured output. You probably want to do that such that different CI agents won't rebuild/retest the same stuff all over again. You could also set up distributed execution where it would split commands across many machines automatically. This would make your CI shorter and faster but this requires you to change your CI config🙂
  • And if you need a quick VC to talk about anything, I'm happy to do it.

@netlify
Copy link

netlify bot commented Jun 13, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 5af467a
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62d02a369bf182000865631d

@vsavkin vsavkin force-pushed the use-nx branch 3 times, most recently from ee553f2 to 0d9745a Compare June 13, 2022 18:44
@virtuoushub
Copy link
Collaborator

👋🏻 @vsavkin - thanks for the contribution! I started work on this over at #5651 and noticed some of what you mentioned in your "I made some changes to the build." sections, but did not get around to implementing any fixes. I will close my PR, since your's is in a more mature state.

@dac09 dac09 added the release:chore This PR is a chore (means nothing for users) label Jun 14, 2022
nx.json Outdated
},
"pluginsConfig": {
"@nrwl/js": {
"analyzeSourceFiles": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jtoar jtoar self-assigned this Jun 20, 2022
@dac09 dac09 self-assigned this Jun 20, 2022
@dac09 dac09 removed their assignment Jun 27, 2022
@Tobbe
Copy link
Member

Tobbe commented Jul 3, 2022

image

Nice!

nx.json Outdated
"test",
"build"
],
"parallel": 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vsavkin is there an easy way to conditionally set this value based upon OS? e.g. in pseudo code "parallel": OS == Windows ? 1 : 5

@jtoar
Copy link
Contributor

jtoar commented Jul 11, 2022

@vsavkin tried resolving merge conflicts for you, but didn't work so I reverted. Sorry about the noise! I think I could've resolved it easily by rebasing but I would've had to force push.

@jtoar jtoar enabled auto-merge (squash) July 14, 2022 14:56
@jtoar jtoar merged commit 53ae908 into redwoodjs:main Jul 14, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 14, 2022
@jtoar jtoar modified the milestones: next-release, v2.2.0 Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

5 participants