Skip to content

Turborepo Implementation & Monorepo standardization#3261

Merged
JacobMGEvans merged 77 commits into
mainfrom
jacobmgevans/turborepo
Jun 12, 2023
Merged

Turborepo Implementation & Monorepo standardization#3261
JacobMGEvans merged 77 commits into
mainfrom
jacobmgevans/turborepo

Conversation

@JacobMGEvans
Copy link
Copy Markdown
Contributor

@JacobMGEvans JacobMGEvans commented May 17, 2023

👋🏻 This has been a long time in the making #1990, we are finally here! The Repo has undergone a lot of initial growth and evolution as we worked to make the migration happen to TypeScript and further provide better development experiences for our users, this is now us doing the same for ourselves which will enable us to evolve AGAIN!

Why TurboRepo & Monorepo?

The goal of providing an architecture & system that decouples the Wrangler core so that it can be used in external modules, we would like to move the workers-sdk towards a Monorepo structure and manage it with a modern tooling built for Monorepos like Turborepo.

This would help us standardize and enforce Monorepo conventions. Additionally packages will share a core config, i.e. TSConfig & ESLint config, while also extending that to their own isolated config and environments, this will prevent global configs becoming increasing complex and potentially conflicts between local configs that don't necessarily need the global config.

This move will help to reduce build times and test failures by enabling caching of builds and by intelligently running tests and other checks only on parts of the codebase that have changed or are dependent on each other.

Future improvements & things this PR facilitates:

  • Turborepo codegen for spinning up and wiring up new packages or other templateable things Docs
  • Remote caching of builds, tests, checks, etc... which will be built custom using TurboRepo Remote Cache API Docs
  • Further isolation and decentralizing of configurations and the packages themselves, i.e. Wrangler getting broken into wrangler/cli, wrangler/core, etc...
  • We will need to follow up this work with specific configuration on how we want to handle Templates, some templates have VERY different local setups & config than the rest of workers-sdk due to the nature of them being created outside of the workers-sdk repo and migrated in later
  • TypeScript Composition setup for Projects in monorepo

Conclusion

Overall, this is a move to improve the developer experience of a repo that many teams contribute to on a daily basis.

Comparisons

CI/CD Checks TurboRepo Checks time (Linting, Types & Formatting combined in parallel) Check time 3 minutes

Before TurboRepo
check time 7 minutes

Local Terminal Runs TurboRepo Checks - uncached (Linting, Types & Formatting combined highly parallel) Approximately 17 seconds

TurboRepo Checks - Cached (Linting, Types & Formatting combined)
Approximately 500-700 milliseconds

Before TurboRepo - Linting, Types & Formatting concurrent w/ no caching comparison
Approximately 1 minute

Special Thanks

@anthonyshew - https://twitter.com/anthonysheww

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 17, 2023

⚠️ No Changeset found

Latest commit: df8b510

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 17, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5245269842/npm-package-wrangler-3261

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3261/npm-package-wrangler-3261

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5245269842/npm-package-wrangler-3261 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5245269842/npm-package-cloudflare-pages-shared-3261

Note that these links will no longer work once the GitHub Actions artifact expires.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2023

Codecov Report

Merging #3261 (df8b510) into main (2ce2423) will decrease coverage by 0.07%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3261      +/-   ##
==========================================
- Coverage   75.42%   75.36%   -0.07%     
==========================================
  Files         182      183       +1     
  Lines       11070    11022      -48     
  Branches     2903     2893      -10     
==========================================
- Hits         8350     8307      -43     
+ Misses       2720     2715       -5     
Impacted Files Coverage Δ
packages/wrangler/src/config/index.ts 90.38% <ø> (ø)
...kages/wrangler/src/constellation/createProject.tsx 83.33% <ø> (ø)
...s/wrangler/src/constellation/deleteProjectModel.ts 91.30% <ø> (ø)
packages/wrangler/src/constellation/listModel.tsx 93.75% <ø> (ø)
...ackages/wrangler/src/constellation/uploadModel.tsx 84.61% <ø> (ø)
packages/wrangler/src/constellation/utils.ts 93.18% <ø> (ø)
packages/wrangler/src/d1/backups.tsx 29.11% <ø> (ø)
packages/wrangler/src/d1/create.tsx 31.25% <ø> (ø)
packages/wrangler/src/d1/delete.ts 34.78% <ø> (ø)
packages/wrangler/src/d1/execute.tsx 55.10% <ø> (ø)
... and 23 more

... and 7 files with indirect coverage changes

@JacobMGEvans JacobMGEvans force-pushed the jacobmgevans/turborepo branch 3 times, most recently from 40bd1ee to 500d896 Compare May 23, 2023 15:12
@JacobMGEvans JacobMGEvans changed the title Jacobmgevans/turborepo Turborepo & Monorepo standardization May 23, 2023
@JacobMGEvans JacobMGEvans added maintenance Maintenance task chunky Bigger than most labels May 23, 2023
@JacobMGEvans JacobMGEvans marked this pull request as ready for review May 30, 2023 14:11
@JacobMGEvans JacobMGEvans requested a review from a team May 30, 2023 14:11
Comment thread .prettierignore Outdated
Comment thread fixtures/pages-simple-assets/tests/index.test.ts Outdated
Comment thread fixtures/pages-functions-wasm-app/tests/index.test.ts Outdated
Comment thread package.json Outdated
Comment thread packages/eslint-workers-config/index.js Outdated
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Looking good. I asked a couple of clarifying questions before approving.

Comment thread test.md Outdated
Comment thread fixtures/external-durable-objects-app/package.json Outdated
Comment thread fixtures/d1-worker-app/package.json Outdated
Comment thread fixtures/no-bundle-import/package.json Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test:watch": "npx vitest"
"test:watch": "vitest"

??

Copy link
Copy Markdown
Contributor Author

@JacobMGEvans JacobMGEvans May 31, 2023

Choose a reason for hiding this comment

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

vitest run will never watch and vitest will watch in local dev mode. TurboRepo will hang sometimes with vitest. Additionally npx is unnecessary, everything should be a dependency in the packages as a Monorepo standard convention.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah this comment was about the fact that there is inconsistent use (or non-use) of npx in these scripts. If a tool is installed locally (which it should be!) then npx should not be needed - for wrangler, vitest, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree there should be a consistent enforcement.

Comment thread fixtures/node-app-pages/package.json Outdated
Comment thread fixtures/pages-d1-shim/package.json Outdated
Comment thread fixtures/pages-simple-assets/tests/index.test.ts Outdated
Comment thread packages/quick-edit/package.json Outdated
Comment thread package.json Outdated
@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jun 1, 2023

Running locally, npm run test fails the local-mode-tests - they timed-out and then turbo appears to hang.

I did a git clean and npm ci just to be sure, and then npm run test failed on edge-preview-authenticated-proxy:test, which said it didn't have wrangler build available.

So I added "dependsOn": ["^topological"], to the test section in turbo.json, and it then failed with the same error on local-mode-tests:test.

Then I tried just running it again and now it failed on node-app-pages:test with a timeout.

I think we need to ensure that all these tests can run successfully from a clean clone and install of the project before we land this. It feels like there is still some configuration that is missing?

(It may be that the timeouts are due to the workerd permission pop-ups blocking the tests; and if you don't click accept quickly enough then the tests timeout?)

Comment thread fixtures/pages-simple-assets/tests/index.test.ts Outdated
Comment thread packages/prerelease-registry/package.json Outdated
Comment thread fixtures/pages-plugin-mounted-on-root-app/tests/index.test.ts
Comment thread packages/eslint-workers-config/package.json Outdated
Comment thread packages/workers-tsconfig/package.json Outdated
Copy link
Copy Markdown
Contributor

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

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

oh damn

I have reviewed the code relevant to Pages, as well as the global-level stuff and smatterings not relevant to Pages. Pretty much all good to me. One tiny tiny nit and a suggestion about package naming (which isn't strictly relevant to this PR, but you do add some new packages which fall under have the same 'problem').

LGTM, thanks @JacobMGEvans! ✅

@JacobMGEvans
Copy link
Copy Markdown
Contributor Author

Current status locally.
Screenshot 2023-06-09 at 3 29 52 PM
Screenshot 2023-06-09 at 3 30 03 PM

@JacobMGEvans JacobMGEvans force-pushed the jacobmgevans/turborepo branch from f0cee63 to 5f6e333 Compare June 9, 2023 23:39
@JacobMGEvans JacobMGEvans merged commit 05099f4 into main Jun 12, 2023
@JacobMGEvans JacobMGEvans deleted the jacobmgevans/turborepo branch June 12, 2023 17:57
@anthonyshew
Copy link
Copy Markdown

🥳 🥳 🥳

@JacobMGEvans JacobMGEvans restored the jacobmgevans/turborepo branch June 12, 2023 20:13
@JacobMGEvans JacobMGEvans deleted the jacobmgevans/turborepo branch August 15, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chunky Bigger than most maintenance Maintenance task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants