refactor: use a shared package - #80
Conversation
634a8a5 to
59c74fb
Compare
68b662b to
de04376
Compare
efef689 to
0bc3533
Compare
0bc3533 to
51f6a06
Compare
|
@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. |
| * - 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. |
There was a problem hiding this comment.
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)
| import { access } from 'node:fs/promises' | ||
|
|
||
| /** Conditional check whether a file exists */ | ||
| export async function existsAsync(p: string): Promise<boolean> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
also I notice that this function is merely moved not created by this PR, so no need to fixate on it.
There was a problem hiding this comment.
To this end, I made the docstring a bit more pointed in 3c758e7
jcreedcmu
left a comment
There was a problem hiding this comment.
The goal of moving functionality to a common place to be shared by different bits of workbench seems desirable.
…ile existing after the function returns
7296b60 to
3c758e7
Compare
This PR moves some code into a different
sharedNPM workspace, which is itself separated into two exports:@leanprover/workbench-sharedthat can run in any setting, and@leanprover/workbench-shared/nodewhich 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 likeimport { 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 requireserasableSyntaxOnly,verbatimModuleSyntaxandmoduleResolution: "nodenext", which enforce type strippability.)The refactor is independently useful because it allows utility functions to be shared between Next.js and the
collab-serverandvscode-workbenchsubprojects — 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.