Skip to content
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

chore: Cleanup duplication across tsconfig files in packages #30764

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

jennifer-shehane
Copy link
Member

@jennifer-shehane jennifer-shehane commented Dec 13, 2024

Additional details

This updates the base package/ts/tsconfig.json file to contain the commented config of TypeScript 5.3 as a base (no config changes) and removes duplication from other package tsconfig files that extend from it. THERE SHOULD BE NO ACTUAL CHANGES TO CODE from this. There were a couple of packages that I added extensions from the base tsconfig file though.

*The packages/runner is unfortunately a special case and things blew up when I tried to extend the tsconfig there. It will require a separate PR to reconcile.

Steps to test

Repo should build/pass tests as before.

How has the user experience changed?

None

PR Tasks

@@ -1,65 +1,118 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main file that most of the other tsconfigs are extending off of. There should be no changes to the configuration of this file. I updated the comments to match TS 5.3 config options and defaults since the older version was incorrect in some areas.

@jennifer-shehane jennifer-shehane self-assigned this Dec 13, 2024
@@ -1,55 +1,12 @@
{
"extends": "../ts/tsconfig.json",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newly extended

@@ -1,12 +1,7 @@
{
"extends": "../ts/tsconfig.json",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newly extended

export type CyCookie = Pick<chrome.cookies.Cookie, 'name' | 'value' | 'expirationDate' | 'hostOnly' | 'domain' | 'path' | 'secure' | 'httpOnly'> & {
// use `undefined` instead of `unspecified`
sameSite?: 'no_restriction' | 'lax' | 'strict'
}

// Cypress uses the webextension-style filtering
// https://developer.chrome.com/extensions/cookies#method-getAll
// @ts-ignore
Copy link
Member Author

@jennifer-shehane jennifer-shehane Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't quite understand why these server checks weren't failing before. Here ts cannot find 'chrome'

@@ -125,6 +125,7 @@ export = {
show: true,
frame: true,
transparent: false,
// @ts-ignore
Copy link
Member Author

@jennifer-shehane jennifer-shehane Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot find getPathToIcon - I think there's something strange with the icons package export/import, but don't think this should be a part of this PR

Copy link

cypress bot commented Dec 13, 2024

cypress    Run #58874

Run Properties:  status check passed Passed #58874  •  git commit 27bf401135: Add ts-ignore
Project cypress
Branch Review tsconfig-cleanup
Run status status check passed Passed #58874
Run duration 24m 16s
Commit git commit 27bf401135: Add ts-ignore
Committer Jennifer Shehane
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 1326
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 29400
View all changes introduced in this branch ↗︎
UI Coverage  46.11%
  Untested elements 189  
  Tested elements 166  
Accessibility  92.54%
  Failed rules  3 critical   8 serious   2 moderate   2 minor
  Failed elements 905  

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. I am not the biggest fan of the shared ts config throughout the repo as it makes updating individual packages more difficult but I am a bigger fan of consistency so at least it's now consistent!

@jennifer-shehane
Copy link
Member Author

@AtofStryker What is more difficult to update with this pattern?

@AtofStryker
Copy link
Contributor

@AtofStryker What is more difficult to update with this pattern?

if there needs to be more specific changes to one ts config vs another, which you could always override, but eventually it needs to be unified and usually winds up in a similar state we are in today which I think defeats the purpose.

@jennifer-shehane jennifer-shehane merged commit 68c5714 into develop Dec 16, 2024
84 of 85 checks passed
@jennifer-shehane jennifer-shehane deleted the tsconfig-cleanup branch December 16, 2024 16:12
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 17, 2024

Released in 13.17.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.17.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Dec 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants