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: [DX-3280] Fix missing dependencies, flaky configs, tests and update NX parallel count #2220

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

zaidarain1
Copy link
Contributor

@zaidarain1 zaidarain1 commented Sep 24, 2024

Some packages import from dependencies without having them listed in their dependencies in their respective package.jsons. Add these to the package.json dependencies as needed.

Some packages have configs and/or tests that are sensitive to breakage since they aren’t explicit or don't handle certain situations. Update these to be more resilient and handle any workflows that can introduce issues (for eg .yalc artifacts resulting in problems with jest)

Deprecated examples have also upgraded typescript to the matching 5.6.2 as the rest of the repo

@zaidarain1 zaidarain1 requested review from a team and shineli1984 as code owners September 24, 2024 05:43
Copy link

nx-cloud bot commented Sep 24, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 242b7f5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

@zaidarain1 zaidarain1 changed the title chore: [DX-3280] Fix missing dependencies, flaky configs and tests chore: [DX-3280] Fix missing dependencies, flaky configs, tests and update NX parallel count Sep 24, 2024
@@ -62,7 +68,7 @@
"lint:fix": "cd ../../../.. && yarn wsrun -p @imtbl/dex-sdk -c lint --fix",
"test": "jest test",
"test:watch": "jest --watch",
"typecheck": "tsc --noEmit"
"typecheck": "tsc --customConditions default --noEmit"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recently live types were added, so you can have live typechecking across packages without needing to build/save, as if the code lives within one monolithic package. We use custom export conditions to enable that behaviour, while also allowing us to switch back to declaration files when building for publishing.

The problem we can face sometimes is without falling back to the declaration files for types from children workspace packages when doing a full typecheck during build or via the above typechecking command, it can produce errors as the parent tsconfig might not be be configured correctly to handle the children package's ts files, one example being jsx/tsx. This just ensures that we fallback to the default conditions correctly during the typechecking process. The only time we should be using the src .ts files directly for types is for the ts server in the IDE to provide cross package live types.

This gives us the best of both worlds.

@@ -23,7 +23,7 @@ try {

if (isDependent || changedProject === currentProject) {
// Rebuild the current project
const command = `nx run-many --target=d --projects=${currentProject} --parallel=5 --no-cloud`;
const command = `nx run-many --target=d --projects=${currentProject} --no-cloud`;
Copy link
Contributor

Choose a reason for hiding this comment

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

why no-cloud for this? is it to skip caching hit?

Copy link
Contributor

Choose a reason for hiding this comment

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

reduce parallel count for flakiness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no-cloud here will keep local cache, but skip the remote cache. With the dev mode being used mostly for constant changes, trying to hit the cloud again resulted in wasted resources and added some time to the dev rebuild loop. The no-cloud was added a bit ago to save the few seconds that were being wasted hitting the cloud at the start of each dev loop unnecessarily.

The parallel count was just moved to a common value in the nx.json rather than being hard coded everywhere, and was bumped to 8 to maximize the 8 core usage of our runners in the more workload heavy workflows 😄

@zaidarain1 zaidarain1 added this pull request to the merge queue Sep 24, 2024
Merged via the queue into main with commit 8b1ceba Sep 24, 2024
13 checks passed
@zaidarain1 zaidarain1 deleted the chore/DX-3280-fix-deps-configs-tests branch September 24, 2024 08:06
@zaidarain1 zaidarain1 self-assigned this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants