Skip to content

Conversation

@nicknovitski
Copy link
Member

@nicknovitski nicknovitski commented Jun 19, 2025

Why

It's simpler, and usually faster (ctix is slower since it doesn't implement parallelization).

How

All existing commands are supported except for yarn ctix in packages to rebuild the barrel file only of that package.

typescript

Use project references. A root tsconfig.json lists all the packages as references, each package's tsconfig.json extends the root tsconfig.base.json, and lists the workspace packages it depends on as references.

In the root, yarn build builds all package which have out-of-date outputs. In a package, yarn tsc builds that project, and yarn build builds it and any packages it depends on.

Because of interactions with ctix, the packages' tsconfig files specify ["src/**/*"] as their input, and . as their rootDir, meaning their output changes from build to build/src.

These commands check and build all files, including tests, so instead of a separate set of "build" tsconfig files, the test files are added to .npmignore, so they aren't included in published packages.

eslint

Pretty straightforward, although the root yarn lint command now lints all matching files in the repo.

tests

From root, yarn test and integration start one jest process which runs all tests. Since a test will fail if its packaged dependencies aren't built, these commands run yarn build:all --noCheck first. With project references, this only builds out of date projects, and extremely fast when none of them are out of date.

Individual package test and integration commands are equivalent to running yarn <command> <path to a package>.

ctix

Changing the root .ctirc file to reference each package, and changing the preface script to read the packages' names from their package.json.

Test Plan

I ran all these commands a bunch.

The coverage decreasing is from fixing an error in the way we were tracking coverage previously, note this part in the summary:

  Files                  77       89      +12  

So after this commit, codecov included 12 more files in its coverage. I'm guessing these are files that are not loaded in the tests of their own package, but are in other packages.

Copy link
Member Author

nicknovitski commented Jun 19, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codecov
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (93d8480) to head (398852e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #297   +/-   ##
=======================================
  Coverage   97.39%   97.39%           
=======================================
  Files          87       87           
  Lines       11889    11889           
  Branches      554      629   +75     
=======================================
  Hits        11579    11579           
  Misses        303      303           
  Partials        7        7           
Flag Coverage Δ
integration 5.98% <ø> (-91.41%) ⬇️
unittest 94.26% <ø> (-3.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nicknovitski nicknovitski marked this pull request as ready for review June 19, 2025 20:44
@nicknovitski nicknovitski requested a review from wschurman as a code owner June 19, 2025 20:44
@wschurman wschurman changed the title Unify repo-wide scripts chore: Unify repo-wide scripts Jun 20, 2025
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

yarn integration and yarn typedoc seem to fail for me locally. I think once typedoc succeeds, yarn lint also fails since those files aren't excluded, but this one is hard to verify since I can't run typedoc.

Copy link
Member

Re: codecov. Based on a spot check, it looks like something is wrong with the new setup though it's hard to tell what. For example, from the report (https://app.codecov.io/gh/expo/entity/pull/297/indirect-changes) it says that L455 of packages/entity/src/EntityPrivacyPolicy.ts is uncovered, but there is a test that tests for that exact string error message so it must be covered? "Invalid RuleEvaluationResult returned from rule"

Copy link
Member Author

nicknovitski commented Jun 20, 2025

yarn integration and yarn typedoc seem to fail for me locally. I think once typedoc succeeds, yarn lint also fails since those files aren't excluded, but this one is hard to verify since I can't run typedoc.

Ah, I deleted the empty entity-codemode/src/index.ts, and that makes typedoc error. That's an easy fix.

I'll look further at the coverage differences. What error do you get with integration? e: and I saw your other comment, I'll fix that too.

@nicknovitski nicknovitski changed the base branch from tsconfig-bases to graphite-base/297 June 20, 2025 19:20
@graphite-app graphite-app bot changed the base branch from graphite-base/297 to main June 20, 2025 19:25
@nicknovitski nicknovitski changed the base branch from main to graphite-base/297 June 20, 2025 20:04
@nicknovitski nicknovitski changed the base branch from graphite-base/297 to 06-20-chore_explicitly_list_files_to_collect_coverage_from June 20, 2025 20:04
@nicknovitski nicknovitski changed the base branch from 06-20-chore_explicitly_list_files_to_collect_coverage_from to graphite-base/297 June 20, 2025 20:38
@nicknovitski nicknovitski changed the base branch from graphite-base/297 to 06-20-chore_use_v8_coverage_provider June 20, 2025 20:38
@nicknovitski nicknovitski changed the base branch from graphite-base/297 to 06-20-chore_explicitly_list_files_to_collect_coverage_from June 20, 2025 22:10
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Few more issues I'm running into when testing this:

  • When I run yarn test from a subpackage directory (packages/entity for example), I believe it is running all the tests from all packages? It should only run the tests from that package if possible.
  • When I run yarn test src/__tests__/EnforcingEntityLoader-test.ts from the packages/entity directory it also runs all the tests. It should only run that test if possible.
  • Ditto with integration tests.

Copy link
Member

(rest of the changes look good though!)

@nicknovitski
Copy link
Member Author

Few more issues I'm running into when testing this:

* When I run `yarn test` from a subpackage directory (`packages/entity` for example), I believe it is running all the tests from all packages? It should only run the tests from that package if possible.

* When I run `yarn test src/__tests__/EnforcingEntityLoader-test.ts` from the `packages/entity` directory it also runs all the tests. It should only run that test if possible.

* Ditto with integration tests.

Ah, yep. Fixed now.

@nicknovitski nicknovitski changed the base branch from 06-20-chore_explicitly_list_files_to_collect_coverage_from to graphite-base/297 June 23, 2025 22:45
@graphite-app graphite-app bot changed the base branch from graphite-base/297 to main June 23, 2025 22:46
@nicknovitski nicknovitski force-pushed the root-scripts branch 3 times, most recently from f00539d to cd9cf6c Compare June 23, 2025 23:10
@nicknovitski nicknovitski force-pushed the root-scripts branch 5 times, most recently from a282900 to 1bf6c53 Compare June 24, 2025 00:20
These are faster, and can pass arbitrary arguments to the underlying
command, which allows repo-wide "watch" workflows.
Copy link
Member Author

I finally fixed the coverage problem: I was not uploading all of the files which codecov uses! The total coverage is now consistent.

@nicknovitski nicknovitski requested a review from wschurman June 24, 2025 00:31
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

awesome stuff

Copy link
Member Author

nicknovitski commented Jun 24, 2025

Merge activity

  • Jun 24, 4:58 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 24, 4:58 PM UTC: @nicknovitski merged this pull request with Graphite.

@nicknovitski nicknovitski merged commit 18a0d29 into main Jun 24, 2025
4 checks passed
@nicknovitski nicknovitski deleted the root-scripts branch June 24, 2025 16:58
@nicknovitski nicknovitski mentioned this pull request Jun 25, 2025
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