-
Notifications
You must be signed in to change notification settings - Fork 3.4k
chore: convert additional server files to TypeScript #32950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
cypress
|
||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
chore/server-files-p3
|
| Run status |
|
| Run duration | 18m 23s |
| Commit |
|
| Committer | Bill Glesias |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
1
|
|
|
72
|
|
|
0
|
|
|
5641
|
| View all changes introduced in this branch ↗︎ | |
Warning
Partial Report: The results for the Application Quality reports may be incomplete.
Warning
No Report: Something went wrong and we could not generate a report for the Application Quality products.
0d1dd13 to
f262fa7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Silent Encoding Failure in `readJson` Calls
The fs.readJsonAsync call passes 'utf8' as a second parameter, but fs-extra's readJson/readJsonSync methods only accept an options object as the second parameter, not an encoding string. This will cause the encoding to be ignored or potentially throw an error. The original code had a @ts-expect-error comment acknowledging this type mismatch, which was removed during the TypeScript conversion without fixing the underlying issue.
packages/server/lib/util/file.ts#L161-L162
cypress/packages/server/lib/util/file.ts
Lines 161 to 162 in 32aa387
| return fs.readJsonAsync(this.path, 'utf8') |
32aa387 to
1fea56c
Compare
0d5411e to
1fd4bad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: `readJsonAsync` Parameter Type Mismatch
The fs.readJsonAsync call passes 'utf8' as a second parameter, but fs-extra's readJson method expects an options object, not a direct encoding string. When promisified by Bluebird, readJsonAsync should be called as fs.readJsonAsync(this.path, { encoding: 'utf8' }) or simply fs.readJsonAsync(this.path) since utf8 is the default encoding. This will cause the method to fail or behave unexpectedly.
packages/server/lib/util/file.ts#L161-L162
cypress/packages/server/lib/util/file.ts
Lines 161 to 162 in 03ffbf0
| return fs.readJsonAsync(this.path, 'utf8') |
|
|
||
| // needed to run these tests locally | ||
| // sinon.stub(ctx.browser, 'machineBrowsers').resolves([ | ||
| // { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually when we convert these tests to vitest we need to address this problem, as the tests don't run locally without this stub but run fine in CI because electron is installed
03ffbf0 to
0989d19
Compare
convert fixture file to TS fix types remove only
0989d19 to
7db621a
Compare
jennifer-shehane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, fine to merge
| // @ts-expect-error | ||
| return fixtureGet(config.fixturesFolder, filePath, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a contextual comment for this expect-error?
| // @ts-expect-error | ||
| const colors = <string[]>[].concat(options.color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add context to this comment?
| export async function readFiles (projectRoot: string, options: { files: { path: string, encoding?: BufferEncoding }[] } = { files: [] }) { | ||
| const files = await Promise.all(options.files.map(async (file) => { | ||
| const { contents, filePath } = await readFile(projectRoot, { | ||
| file: file.path, | ||
| encoding: file.encoding, | ||
| }) | ||
|
|
||
| return { | ||
| ...file, | ||
| filePath, | ||
| contents, | ||
| } | ||
| })) | ||
|
|
||
| return files | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a direct translation, but just wondering if this should have a try/catch here.
| // @ts-expect-error | ||
| err.code = 'ENOENT' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment on this expect-error?
|
|
||
| export async function parseFile (p: string, fixture: string, options: { encoding?: (ObjectEncodingOptions & { flag?: string | undefined }) | BufferEncoding | null } = {}) { | ||
| if (queue[p]) { | ||
| await Bluebird.delay(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Bluebird here exactly?
| const dc = process.env.NODE_DISABLE_COLORS | ||
|
|
||
| process.env.NODE_DISABLE_COLORS = '0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the zero context on why this is necessary 😆
| export async function parseHtml (p: string, fixture: string) { | ||
| try { | ||
| const content = await fs.readFileAsync(p, 'utf8') | ||
|
|
||
| return content | ||
| } catch (err) { | ||
| throw new Error(`Unable to parse '${fixture}'.\n${err.toString()}`) | ||
| } | ||
| } | ||
|
|
||
| export async function parse (p: string, fixture: string, encoding: (ObjectEncodingOptions & { flag?: string | undefined }) | BufferEncoding | null | undefined) { | ||
| try { | ||
| const content = await fs.readFileAsync(p, encoding) | ||
|
|
||
| return content | ||
| } catch (err) { | ||
| throw new Error(`Unable to parse '${fixture}'.\n${err.toString()}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems these can be consolidated, but maybe another time.
Additional details
Converts files, fixtures, resolve, status_code, system, and a few other files to TypeScript
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?Note
Migrates key server modules (files, fixture, terminal, tty, resolve, status_code, system) and related tests to TypeScript, updates call sites/imports and typings, and adds safe error serialization.
util/{terminal.ts, tty.ts, resolve.ts, status_code.ts, system.ts}to TypeScript; adjust consumers toimport * as ...where needed.fs.readJsonAsynctoutil/fs.ts; remove legacy TS ignores inutil/file.ts.server-base.ts,project-base.ts,modes/run.tsto use typed utilities and minor typing tweaks.lib/files.jsandlib/fixture.jswithlib/files.tsandlib/fixture.ts; update usages insocket-base.ts,controllers/xhrs.ts, andprivileged-commands-manager.ts(get as fixtureGet,* as files).cloud/exception.ts: switch to* as systemimport and addsafeErrorSerialize()for robust error stringification.start-cypress.js: importoverridefromutil/ttyand call directly.util/print-run.ts: type fixes (e.g.,HorizontalAlignment) and table alignment updates.files_spec.ts,fixture_spec.ts,status_code_spec.ts,util/terminal_spec.ts,util/tty_spec.ts) and update snapshots.NODE_OPTIONS='--import tsx'for TS config loading.Written by Cursor Bugbot for commit 7db621a. This will update automatically on new commits. Configure here.