Skip to content

Conversation

cloutiertyler
Copy link
Contributor

@cloutiertyler cloutiertyler commented Aug 21, 2025

Description of Changes

This PR:

  • standardizes the prettier config across all TypeScript projects
  • adds a root level package.json
  • standardizes all pnpm commands to be the same
  • updates documentation accordingly
  • adds some additional typescript testing for serialization and deserialization

IMPORTANT! Once this PR merges we will need to change the compile-and-test required check to build-and-test

API and ABI breaking changes

No breaking changes.

Expected complexity level and risk

2 - It in principle doesn't change any code, but could affect deploy processes.

Testing

  • Just the automated testing that we had previously
  • I added additional automated tests

@cloutiertyler cloutiertyler changed the base branch from master to tyler/ts-module-test August 21, 2025 23:12
@cloutiertyler cloutiertyler marked this pull request as ready for review August 22, 2025 17:02
@@ -8,7 +8,7 @@ on:
merge_group:

jobs:
compile-and-test:
build-and-test:
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI I believe this job name is used in the required status checks, so we will have to update the required name after this merges

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add that note into the PR description somewhere so it's a bit easier to remember?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, yes I can

@@ -6,15 +6,15 @@ For each table containing an `#[auto_inc]` column, SpacetimeDB creates a sequenc

### How It Works

* Sequences in SpacetimeDB use Rust’s `i128` integer type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these docs tweaks are important to you, can you please move them into a separate PR? They seem entirely unrelated to the (meaningfully large) actual goal of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I typically do something like:

git checkout -b new-branch origin/master
git checkout old-branch -- file1 file2 ...
git commit -am 'Moving files from other thing'
git push -u
gh pr create --web

Copy link
Collaborator

@bfops bfops Aug 22, 2025

Choose a reason for hiding this comment

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

Ah is this because we started running pnpm format on the docs? That definitely seems like a separate change, since the docs are entirely unrelated to our TypeScript bindings/SDK.

By default I would instead lean towards adding the docs to .prettierignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is because I'm running the same pnpm format on all typescript directories (directories with package.json). I can exempt markdown from the formatting and add that in a separate PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think that eventually we should have docs under prettier so that we don't have non-substantive formatting changes to the docs causing diffs in the future.

Copy link
Contributor Author

@cloutiertyler cloutiertyler Aug 22, 2025

Choose a reason for hiding this comment

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

I've added **/*.md to the docs .prettierignore. Notably the cli-reference.md file formatting is still changed because I moved the not portable sed scripts into a node js script which rewrites the file with remark.

@@ -0,0 +1,99 @@
#!/usr/bin/env node
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 script replaces the non-portable sed commands that were used to fix up the markdown.

Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

Approving the docs formatting + CI changes, with the caveat that I think none of them should actually be happening in this PR (because I don't see the docs directory as a typescript project, or at the very least not the same kind of typescript project as the ones that are related to actual SpacetimeDB implementation). But if I close my eyes and pretend that this PR is only making those changes, they seem good to me.

"unified": "^11.0.5",
"unist-util-visit": "^5.0.0"
},
"scripts": {
"build": "tsc --project ./tsconfig.json",
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown && prettier --write docs/nav.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should a formatting step really be done during build?

Suggested change
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown && prettier --write docs/nav.js",
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown",

@cloutiertyler cloutiertyler force-pushed the tyler/unify-ts-projects branch from 13b4996 to 4cca832 Compare August 25, 2025 16:23
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