Refactor Hocuspocus server core to use Web Platform APIs, convert to Vitest, and remove any types#3
Refactor Hocuspocus server core to use Web Platform APIs, convert to Vitest, and remove any types#3
any types#3Conversation
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
|
@copilot let's also convert the project to use vitest as the unit test runner and convert the tests already in the tests directory and elsewhere to use vitest. We will use the tests directory to store the spec files next to the code. The test file name convention is .spec.ts Add GitHub actions file that runs for all pull requests to the master branch. It should lint the files and then execute the unit tests. Print coverage summary because later we will enforce coverage |
|
@copilot add cursorrules for testing conventions Remove usage of |
packages/server/src/Hocuspocus.ts
Outdated
| @@ -1,4 +1,3 @@ | |||
| import crypto from "node:crypto"; | |||
| import type { IncomingMessage } from "node:http"; | |||
There was a problem hiding this comment.
@copilot This needs to go away and abstracted out. Runtime specific imports should only happen in one segregated location in the code, in the runtime adapter.
There was a problem hiding this comment.
Fixed in commit e4b50d5. Centralized all Node.js type imports (IncomingMessage, IncomingHttpHeaders, ServerResponse) in types.ts and re-exported them. All core files (Hocuspocus.ts, ClientConnection.ts, Connection.ts, util/getParameters.ts) now import these types from types.ts instead of directly from node:http. Runtime-specific imports are now isolated to types.ts and Server.ts.
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
|
@copilot dont put crypto on globalThis. Nothing should be made global. It looks like crypto is only used for uuid generation. You can just put this implementation in the code base as a utility function uuid() {
let d = new Date().getTime();
return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {
const r = (d + Math.random() * 16) % 16 | 0;
d = Math.floor(d / 16);
return (c === 'x' ? r : (r & 0x3 | 0x8)).toString(16);
});
}
// Usage:
console.log(uuid()); |
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
Partially completed in commit 978cfdf:
Completed in commit 978cfdf:
|
|
|
||
| // Every new connection gets a unique identifier. | ||
| private readonly socketId = crypto.randomUUID(); | ||
| private readonly socketId = globalThis.crypto.randomUUID(); |
There was a problem hiding this comment.
@copilot remove usage of globalThis. Nothing should be global
| /** | ||
| * Helper function to create a new Hocuspocus server for testing | ||
| */ | ||
| export async function createTestServer(options?: Partial<ServerConfiguration>) { |
|
@copilot if there's a way to tell copilot to run lint and test before asking for a review and to fix any errors and failing test using a file to hint it that'd be great. I also want copilot to write unit tests for any new code that is added |
|
@copilot finish all the tasks you have for next steps |
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
any types
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
|
@copilot continue |
|
@copilot continue |
|
@copilot continue |
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
Completed all remaining tasks in commit ba72a28: UUID Utility:
Type Safety:
All PR feedback has been implemented:
|
|
@copilot continue with whatever you were working on before rate-limit was hit. Also, make sure lint passes |
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
All lint errors fixed in commit 20419db: Fixed:
Lint Status: All tasks from PR feedback completed. Ready for review. |
|
@copilot What is the point of |
Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
You're absolutely right - it wasn't being used. Removed in commit b7824b6. The Also removed The actual refactoring that matters (removing Node.js runtime imports, using the uuid utility instead of crypto, type safety improvements) is all still in place and working. |
…nfig Co-authored-by: jchoi2x <2028917+jchoi2x@users.noreply.github.com>
All PR Tasks Completed ✅
Latest Updates
Lint Fixes (Latest Commit):
anytypes in Hocuspocus.ts and Server.tsdefaultContext: any→defaultContext: HookContextcontext?: any→context?: HookContexterror: any→error: unknownwith proper type guardsPromise<any>→Promise<void>...args: any[]→...args: unknown[]noForEach,noBannedTypes,noParameterAssign,noNonNullAssertionRemoved Unused Code:
packages/server/src/runtime/directory (not being used)PHASE1_SUMMARY.md(outdated documentation)Lint Status:
npm run lintpasses for server package with zero errorsanytypes eliminated (~80+ instances replaced)Core Refactoring Completed
Core Changes:
crypto.randomUUID()with customuuid()utility (no runtime dependencies)node:cryptoandnode:urlfrom core filestypes.tsTesting & Infrastructure:
Type Safety:
anytypes with proper TypeScript typesunknownwith type guardsHookContexttypePublic API: 100% backward compatible - all existing usage patterns work unchanged.
Ready for merge.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.