chore: convert cache.js file within server to TypeScript #31471
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/server/lib/cache.ts:95
- Using 'memo.push(path)' returns the new length of the array rather than the updated memo array, which can be confusing. Consider refactoring by pushing the path separately and then returning memo to improve clarity.
return memo.push(path)
cypress
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
server-js-to-ts-cache
|
| Run status |
|
| Run duration | 19m 19s |
| Commit |
|
| Committer | Jennifer Shehane |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
15
|
Pending
|
1232
|
Skipped
|
0
|
Passing
|
32139
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
46.59%
|
|
|---|---|
|
Untested elements
|
183
|
|
Tested elements
|
164
|
Accessibility
92.63%
|
|
|---|---|
|
Failed rules
|
3 critical
8 serious
2 moderate
2 minor
|
|
Failed elements
|
887
|
Tests for review
cypress/e2e/studio/studio.cy.ts • 1 failed test • app-e2e
| Test | Artifacts | |
|---|---|---|
| Cypress Studio > remains in studio mode when the test name is changed on the file system and file watching is disabled |
Test Replay
Screenshots
|
|
commands/waiting.cy.js • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| ... > errors > throws when waiting for 2nd response to route |
Test Replay
|
|
issues/28527.cy.ts • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| issue 28527 > fails and then retries and verifies about:blank is not displayed |
Test Replay
Screenshots
|
|
commands/waiting.cy.js • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| ... > errors > throws when waiting for 2nd response to route |
Test Replay
|
|
e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| ... > throws when foo cannot resolve |
Test Replay
|
|
issues/28527.cy.ts • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| issue 28527 > fails and then retries and verifies about:blank is not displayed |
Test Replay
Screenshots
|
|
The first 5 flaky specs are shown, see all 15 specs in Cypress Cloud.
There was a problem hiding this comment.
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/server/test/unit/cache_spec.js:202
- [nitpick] Consider refactoring the test to consistently use async/await rather than mixing it with .then chaining. For example, after awaiting cache.removeProjectPreferences, await the subsequent call to __get rather than chaining .then to improve readability.
await cache.removeProjectPreferences(testProjectTitle)
There was a problem hiding this comment.
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
packages/types/src/cache.ts:6
- [nitpick] Consider providing a more specific type than 'any' for PROJECTS_CONFIG to improve type safety.
PROJECTS_CONFIG: Record<string, any>
packages/server/lib/cache.ts:101
- Returning the result of memo.push (a number) could be misleading; consider refactoring to push the path and then explicitly return the memo.
return memo.push(path)
packages/data-context/src/data/coreDataShape.ts:25
- The return type for savedState has changed from using a dedicated SavedStateShape to Partial; ensure that all consumers of this interface are updated accordingly.
savedState?: () => Promise<Partial<AllowedState>>
| @@ -22,7 +22,7 @@ export interface AuthenticatedUserShape { | |||
|
|
|||
| export interface ProjectShape { | |||
| projectRoot: string | |||
| savedState?: () => Promise<Maybe<SavedStateShape>> | |||
| savedState?: () => Promise<Partial<AllowedState>> | |||
There was a problem hiding this comment.
AllowedState is already a Partial - see L#58 of types/src/preferences.ts - so this is redundant
There was a problem hiding this comment.
Addressed here: ce56bd9 (#31471)
| insertProjectToCache (projectRoot: string) { | ||
| return cache.insertProject(projectRoot) | ||
| async insertProjectToCache (projectRoot: string) { | ||
| await cache.insertProject(projectRoot) |
There was a problem hiding this comment.
The original is actually a little bit better, because this wraps the call to insertProject in an extra no-op microtask. The same goes for other methods here that got updated to async/await unnecessarily.
There was a problem hiding this comment.
K, well, I did do this manually, so that's on me 😝
There was a problem hiding this comment.
Addressed here: 7b302d4 (#31471)
| this.originalEnv = originalEnv | ||
|
|
||
| nock.disableNetConnect() | ||
| nock.enableNetConnect(/localhost/) | ||
|
|
||
| // always clean up the cache | ||
| // before each test | ||
| return cache.remove() | ||
| await cache.remove() |
There was a problem hiding this comment.
Unlike the previous async/await changes, I think this one is good to keep, as it clarifies that the beforeEach is asynchronous.
The logic here is:
- If the function just returns the value that another function returns, and that value is already wrapped in a promise, simply return the execution of that function rather than making the function
asyncandawaiting it. This goes double for functions that only serve to invoke a single async function. - If the function requires the
awaitfor its other operations (like wrapping it in a try/catch, or performing additional operations subsequent to theawait), preferasync/await - always have test hooks and definitions
async/awaiteven if there is only one async call, as readability is preferable over the (slight) performance hit of an unnecessary microtask
packages/server/lib/cache.ts
Outdated
| PROJECTS: [], | ||
| PROJECT_PREFERENCES: {}, | ||
| PROJECTS_CONFIG: {}, | ||
| COHORTS: {}, | ||
| } | ||
| }, | ||
|
|
||
| _applyRewriteRules (obj = {}) { | ||
| _applyRewriteRules (obj: Partial<Cache> = {}): Cache { |
There was a problem hiding this comment.
Copilot is correct about this, but it doesn't go far enough - there are further improvements!
- inlining the convertProjectsToArray and renameSessionToken into _applyRewriteRules
- removing the reduce
- defining a LegacyCache interface and isLegacyCache type guard function
- changing the signature to (cache: LegacyCache) => Cache, lifting the decision to call or not to the caller, and renaming to
convertLegacyCacheToCache
But I don't actually see where _applyRewriteRules is invoked, other than test/unit/cache_spec.js. This may be dead code that can be removed.
There was a problem hiding this comment.
Removal of dead code here: f56e73e (#31471)
packages/server/lib/cache.ts
Outdated
| return this._getProjects(tx).then((projects) => { | ||
| const pathsToRemove = Promise.reduce(projects, (memo, path) => { | ||
| const pathsToRemove = Promise.reduce(projects, (memo: string[], path) => { | ||
| return fs.statAsync(path) | ||
| .catch(() => { | ||
| return memo.push(path) |
There was a problem hiding this comment.
copilot was right in one of its low-confidence comments (probably also omitted because this specific line wasn't modified), but this is a function that could really benefit from unwrapping its promises. Returning memo.push here is, like it said, really weird, as it's not returning a promise that should be awaited, and its return value is otherwise unimportant. (it's overwritten by the .return())
There was a problem hiding this comment.
@cacieprins Yah, Cursor also made this suggestion actually. Updated here: 75e03c9 (#31471)
…to TypeScript (#31471)
…to TypeScript (#31471)
…to TypeScript (#31471)
…to TypeScript (#31471)
…to TypeScript (#31471)
…to TypeScript (#31471)
* chore: updating v8 snapshot cache * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) --------- Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>
* chore: updating v8 snapshot cache * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) --------- Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>
* chore: updating v8 snapshot cache * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) * index on develop: 6b28356 chore: convert cache.js file within server to TypeScript (#31471) --------- Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>
Additional details
Part of converting our repository from JS to TS. This converts the
cache.jsfile in theserverpackage to TypeScript.Steps to test
yarn check-tsHow has the user experience changed?
Hopefully nothing!
PR Tasks
cypress-documentation?type definitions?