chore: convert screenshots.js to screenshots.ts #30758

jennifer-shehane · 2024-12-12T17:12:34Z

Additional details

Converting screenshots.js to screenshots.ts. I was initially looking at this to update our mime dependency and with a lot of deps moving to esm its just easier to upgrade if files are in TS.

I am not a TypeScript expert, so please review each change! 😄 Some stuff I just ignored the TS error, but the code likely needs actual fixing.

Steps to test

Unit/Integration/System tests should pass

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

cypress · 2024-12-12T20:18:59Z

cypress Run #59228

Run Properties: Passed #59228 • 6ee3416e4e: revert outputFile change

Project	`cypress`
Branch Review	`js-to-ts-screenshots`
Run status	`Passed #59228`
Run duration	`24m 32s`
Commit	`6ee3416e4e: revert outputFile change`
Committer	`Jennifer Shehane`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`8`
Pending	`1326`
Skipped	`0`
Passing	`29400`
View all changes introduced in this branch ↗︎

UI Coverage `46.24%`
Untested elements	`188`
Tested elements	`166`

Accessibility `92.54%`
Failed rules	`3 critical` `8 serious` `2 moderate` `2 minor`
Failed elements	`903`

packages/server/lib/screenshots.ts

cacieprins

Looks good - just some suggestions on how to resolve some of the typescript errors!

packages/server/lib/screenshots.ts

cacieprins · 2024-12-13T17:10:02Z

packages/server/lib/screenshots.ts

@@ -350,6 +368,7 @@ const getPath = function (data, ext, screenshotsFolder, overwrite) {
    .chain(data.titles)
    .map(sanitizeToString)
    .join(RUNNABLE_SEPARATOR)
+    // @ts-ignore


I'm not sure what the next line is even doing; _.concat only operates on arrays, and "someString".concat([]) === "someString". The next line can likely be removed, but I'd make sure the values are what they're expected to be if you do.

Ryan had some thoughts on what this is doing here: #30758 (comment)

@cacieprins @ryanthemanuel Yah....it does not like any changes to this area. I'm going to leave it alone. 😬 I don't really understand why it's needed either.

Yeah I think I get what it's doing, but lodash types don't like it even though you can do it.

Hey, while I was working on this part of code in my PR, I found that this is done so that type of the variable is same in both if and else branch. I guess that is why it breaks, as the type for name variable becomes different in case of if and else branches, and the remaining code that uses names breaks on one type or other. I fixed it by replacing the concat by putting it in array.

Relevant lines from my PR https://github.com/cypress-io/cypress/pull/30673/files#diff-471ce7b0d88e3cdc126b6c5e781d074d86a828e5a7d88b9584db3385be09e569R88-R97

packages/server/lib/screenshots.ts

jennifer-shehane · 2024-12-16T14:30:04Z

I'm going to take a closer look at this again.

…s.ts (#30758)

…ts (#30758)

…s.ts (#30758)

…ts (#30758)

…s.ts (#30758)

…ts (#30758)

…s.ts (#30758)

…ts (#30758)

…s.ts (#30758)

…ts (#30758)

…s.ts (#30758)

…ts (#30758)

* chore: updating v8 snapshot cache * index on develop: 0a8a130 chore: convert screenshots.js to screenshots.ts (#30758) * index on develop: 0a8a130 chore: convert screenshots.js to screenshots.ts (#30758) --------- Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>

jennifer-shehane added 2 commits December 12, 2024 12:12

chore: convert screenshots.js to screenshots.ts

54be464

Fix export

70652e5

jennifer-shehane self-assigned this Dec 12, 2024

AtofStryker self-requested a review December 13, 2024 14:51

AtofStryker reviewed Dec 13, 2024

View reviewed changes

packages/server/lib/screenshots.ts Outdated Show resolved Hide resolved