Skip to content

CFSQL-1371 reconcile dumpSql changes from D1 and document how to keep them in sync#10686

Merged
alsuren merged 1 commit intomainfrom
dlaban/CFSQL-1371-dumpSql-reconcile
Oct 8, 2025
Merged

CFSQL-1371 reconcile dumpSql changes from D1 and document how to keep them in sync#10686
alsuren merged 1 commit intomainfrom
dlaban/CFSQL-1371-dumpSql-reconcile

Conversation

@alsuren
Copy link
Contributor

@alsuren alsuren commented Sep 17, 2025

Fixes CFSQL-1371

Describe your change...

This adds a comment at the top of dumpSql.ts so we don't let them get out of sync again. It also adds the d1 stats tracking machinery (which remains unused in miniflare) so that there now only whitespace differences between the two files.


  • Tests
    • Tests included
    • Tests not necessary because: stats tracking functionality is not used by miniflare (is extensively covered by D1's internal test suite)
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: not public facing
  • Wrangler V3 Backport

@alsuren alsuren requested a review from a team as a code owner September 17, 2025 10:36
@changeset-bot
Copy link

changeset-bot bot commented Sep 17, 2025

🦋 Changeset detected

Latest commit: 48e8471

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

This PR includes changesets to release 5 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers 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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 17, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@10686

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@10686

miniflare

npm i https://pkg.pr.new/miniflare@10686

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@10686

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@10686

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@10686

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@10686

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@10686

wrangler

npm i https://pkg.pr.new/wrangler@10686

commit: 48e8471

@alsuren alsuren marked this pull request as draft September 17, 2025 10:40
@dario-piotrowicz
Copy link
Member

I think that this change should actually be backported to wrangler v3 🤔

Wrangler V3 Backport
Not necessary because: no functional change

But there are functional changes here no? 🤔

@dario-piotrowicz
Copy link
Member

(PS: sorry for reviewing the PR already, I only now noticed that it's in draft 😅)

" AND type IN ('index', 'trigger', 'view')",
// 'DESC' appears in the code linked above but the observed behaviour of SQLite appears otherwise
"ORDER BY type COLLATE NOCASE",
].join(" ")
Copy link
Contributor Author

@alsuren alsuren Sep 23, 2025

Choose a reason for hiding this comment

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

This is slightly sad, but D1 uses 2 spaces for indentation and workers-sdk uses tabs. This is the best way I could think of to make both autoformatters produce something that doesn't make your eyes bleed. There is no functional change here.

Copy link
Member

Choose a reason for hiding this comment

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

🥹👍

@alsuren alsuren marked this pull request as ready for review October 6, 2025 13:41
@alsuren alsuren force-pushed the dlaban/CFSQL-1371-dumpSql-reconcile branch from 0610e5e to 48e8471 Compare October 6, 2025 16:45
@alsuren alsuren mentioned this pull request Oct 6, 2025
7 tasks
Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

LGTM 🙂

(For posterity) as discussed internally we agreed with the decision not to include tests here 🙂

The backport is also not super necessary but since (after I asked... sorry about that 🙇) you already got the backport PR ready we can also proceed with that 😄

Thanks @alsuren 🫶

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Oct 6, 2025
@alsuren alsuren merged commit 4462bc1 into main Oct 8, 2025
43 of 47 checks passed
@alsuren alsuren deleted the dlaban/CFSQL-1371-dumpSql-reconcile branch October 8, 2025 10:24
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants