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

DRAFT chore: match dependency versions across the repo #5795

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented May 9, 2024

This isn't ready for review yet, but putting it up for CI to have a look.


tl;dr: makes it so we never use different versions of a package anywhere in the repo. No functional changes.

  • Installs @manypkg/cli (https://github.com/Thinkmill/manypkg)
  • Run manypkg check which returns a list of all the mismatched dependencies
  • Manually fix each dependency failure
  • Run manypkg fix to fix any extra failures (like unordered dependencies in package.jsons)
  • Run pnpm check, pnpm test, pnpm prettify and fix any failures from them
  • Added check:repo npm task that runs manypkg check, and added it to npm run check so it runs in CI

Notes:

  • The most confusing part: when running tests, wrangler/../deploy.tsts.ts and wrangler/../pages/deploy.test.ts fail (on macos and ubuntu respectively). Of note, it appears like runInTmpDir() fails to apply itself, so the tests run in the source directory, and fail expectations. Been fighting this for a week and I can't figure out why or how, yet.
  • These packages hanf when running test:ci on my machine (with node@16.17.1):
    • additional-modules
    • interactive-dev-tests
    • pages-d1-shim
    • pages-workerjs-directory
    • type-generation-fixture
    • worker-app
  • Running tests appears to leave orphaned workerd process
  • I added /templates/* to workspaces and gave it the same treatment, but removed it from workspaces. We'll revisit that folder in the future, I don't want to slow down the builds atm.

Copy link

changeset-bot bot commented May 9, 2024

🦋 Changeset detected

Latest commit: 4ac1066

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
edge-preview-authenticated-proxy Patch
playground-preview-worker Patch
@cloudflare/prerelease-registry Patch
@cloudflare/vitest-pool-workers Patch
workers-playground Patch
create-cloudflare Patch
@cloudflare/kv-asset-handler Patch
turbo-r2-archive Patch
format-errors Patch
@cloudflare/pages-shared Patch
workers.new Patch
@cloudflare/quick-edit Patch
miniflare Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

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

@@ -563,7 +560,7 @@ export const spinner = (
// const frames = ["㊂", "㊀", "㊁"];

const frameRate = 120;
let loop: NodeJS.Timer | null = null;
let loop: ReturnType<typeof setInterval> | null = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

types for timers are a mess across runtimes. This is a cleaner cross runtime compatible way of setting the type. I'm a fan of this format.

@@ -11,7 +11,7 @@ declare module "stream/web" {
}

interface ReadableStreamBYOBReader {
readonly closed: Promise<void>;
readonly closed: Promise<undefined>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating typescript made this a little more strict (I think), so I had to fix this here

@@ -508,6 +508,7 @@ function buildProjectMiniflareOptions(
return {
...SHARED_MINIFLARE_OPTIONS,
unsafeModuleFallbackService: moduleFallbackService,
// @ts-expect-error SUNILFIX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is failing typecheck, will fix before landing

@@ -528,6 +529,7 @@ function buildProjectMiniflareOptions(
return {
...SHARED_MINIFLARE_OPTIONS,
unsafeModuleFallbackService: moduleFallbackService,
// @ts-expect-error SUNILFIX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is failing typecheck, will fix before landing

@threepointone threepointone marked this pull request as ready for review May 9, 2024 15:31
@threepointone threepointone requested review from a team as code owners May 9, 2024 15:31
Copy link
Contributor

github-actions bot commented May 9, 2024

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/9064764843/npm-package-wrangler-5795

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-wrangler-5795 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-create-cloudflare-5795 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-cloudflare-kv-asset-handler-5795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-miniflare-5795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-cloudflare-pages-shared-5795
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9064764843/npm-package-cloudflare-vitest-pool-workers-5795

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


wrangler@3.55.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240419.1
workerd 1.20240419.0 1.20240419.0
workerd --version 1.20240419.0 2024-04-19

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@threepointone threepointone force-pushed the normalize-dependencies branch 4 times, most recently from 5812a50 to 4a890f7 Compare May 10, 2024 08:38
@@ -186,8 +187,10 @@ async function buildPackage() {
"esbuild",
],
plugins: [embedWorkersPlugin],
logLevel: watch ? "info" : "warning",
watch,
logLevel: "warning",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

disabled watch mode here because we updated esbuild and it didn't have the same api. I'll being it back later.

*
* So this helper removes these message from the snapshots to keep them consistent.
*/
export function normalizeProgressSteps(str: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exporting this function and reusing it elsewhere was causing this suite to be run twice, so I extracted it into its own helper.

@@ -22,16 +25,6 @@ function createFetchResult(result: unknown, success = true) {
};
}

export function mockGetMemberships(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just like nomralize-progress-steps, importing this function was causing this suite to be run twice. we already had this helper in /helpers/mock-oauth-flow, so I replaced it with that

@@ -27,16 +30,6 @@ function createFetchResult(result: unknown, success = true) {
};
}

export function mockGetMemberships(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should write a lint rule that prevents exports from .test.ts/.spec.ts files

@@ -180,21 +179,21 @@
"patch-console": "^1.0.0",
"pretty-bytes": "^6.0.0",
"prompts": "^2.4.2",
"react": "^17.0.2",
"react": "^18.3.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a major update

@threepointone threepointone force-pushed the normalize-dependencies branch 12 times, most recently from 164d9e3 to 7f83edf Compare May 10, 2024 16:26
@threepointone threepointone force-pushed the normalize-dependencies branch 4 times, most recently from ba85b02 to ccee1e1 Compare May 10, 2024 23:05
@threepointone threepointone force-pushed the normalize-dependencies branch 9 times, most recently from b03b0de to 1c8d057 Compare May 13, 2024 13:47
@threepointone threepointone force-pushed the normalize-dependencies branch 3 times, most recently from 505539a to edf168f Compare May 13, 2024 14:19
tl;dr: makes it so we never use different versions of a package anywhere in the repo. No functional changes.

- Installs @manypkg/cli (https://github.com/Thinkmill/manypkg)
- Run `manypkg check` which returns a list of all the mismatched dependencies
- Manually fix each dependency failure
- Run `manypkg fix` to fix any extra failures (like unordered dependencies in `package.json`s)
- Run `pnpm check`, `pnpm test`, `pnpm prettify` and fix any failures from them
- Added `check:repo` npm task that runs `manypkg check`, and added it to `npm run check` so it runs in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

None yet

1 participant