Skip to content

refactor: use a shared package - #80

Open
robsimmons wants to merge 7 commits into
mainfrom
push-qvvqkksstkks
Open

refactor: use a shared package#80
robsimmons wants to merge 7 commits into
mainfrom
push-qvvqkksstkks

Conversation

@robsimmons

@robsimmons robsimmons commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

This PR moves some code into a different shared NPM workspace, which is itself separated into two exports: @leanprover/workbench-shared that can run in any setting, and @leanprover/workbench-shared/node which contains shared utilities that only work in nodeJS.

This PR mostly makes sense as preparation for a subsequent refactor that pulls VS Code session management out of next.js and into a separate "shard runner" package. In particular, because I plan to use Node.js Type Stripping for the shard runner, I added allowImportingTsExtensions: true — that permits the shared workspace to have imports that look like import { foo } from "./foo.ts", which is necessary for type stripping, without it tripping up the projects that import the shared workspace. (The shared project also requires erasableSyntaxOnly, verbatimModuleSyntax and moduleResolution: "nodenext", which enforce type strippability.)

The refactor is independently useful because it allows utility functions to be shared between Next.js and the collab-server and vscode-workbench subprojects — polling a filesystem repeatedly until a socket exists is one piece of repeated code that this eliminates, and this PR also shares a bunch of constants that previously weren't synced.

@robsimmons
robsimmons marked this pull request as draft August 11, 2026 23:56
@robsimmons
robsimmons force-pushed the push-qvvqkksstkks branch 2 times, most recently from 68b662b to de04376 Compare August 12, 2026 01:59
@robsimmons
robsimmons force-pushed the push-qvvqkksstkks branch 4 times, most recently from efef689 to 0bc3533 Compare August 12, 2026 15:00
@robsimmons

robsimmons commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

@jcreedcmu @Vtec234 this is a "does this plan for making stuff shared between projects make sense" level code review request — the actual code is basically just copy-paste-and-fixup mechanics and probably doesn't need line-by-line review, the deterministic and nondeterministic robots should be capable of detecting the mistake-in-copy-paste errors and eslint/tsc/pet-robots seem satisfied.

Comment thread shared/shared.ts
Comment on lines +9 to +12
* - Project names are not used in file paths, but may be used in URLs.
* We enforce alphanumeric ASCII names.
* Names are unique up to recasing, natively in the database (`COLLATE NOCASE`).
* Unicode names may be added in the future.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I copy-pasted this from its previous location (actually I changed Names to Project names because we definitely use usernames in file paths). Then the pet robots both alerted to the fact that this is not true — bwrapProjectDir, for example, uses /workspace/projectName as the project directory, which makes sense from the user's perspective but could cause issues if we added unicode names. I believe we only use project names in file paths inside the bubblewrap.

(CC @Vtec234 since you added the previous version of this note in #36)

@robsimmons
robsimmons marked this pull request as ready for review August 12, 2026 18:20
Comment thread shared/node/index.ts
import { access } from 'node:fs/promises'

/** Conditional check whether a file exists */
export async function existsAsync(p: string): Promise<boolean> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The deprecation of fs.exists() is news to me that I discovered by trying to search around for why you had written this function, and I feel a little skeptical that nodejs has really offered the optimal api surface if we find ourselves wanting to write wrappers like this.

But... this is not a request for any change to this PR. I'm just noting this for the historical record. The only thing to watch out for is if we are in fact introducing any races by checking for existence and doing something assuming existence that might be invalidated by a concurrent process deleting it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(and I think I'm inclined to be somewhat tolerant of races that are theoretically possible but practically extremely unlikely, as long as they have relatively benign consequences in the unlikely case)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also I notice that this function is merely moved not created by this PR, so no need to fixate on it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

To this end, I made the docstring a bit more pointed in 3c758e7

@jcreedcmu jcreedcmu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The goal of moving functionality to a common place to be shared by different bits of workbench seems desirable.

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.

2 participants