chore: enforce 'no-implicit-dependencies' on tslint #31022
Conversation
cypress
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
no-implicit-deps
|
| Run status |
|
| Run duration | 18m 25s |
| Commit |
|
| Committer | Jennifer Shehane |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
7
|
Pending
|
1099
|
Skipped
|
0
|
Passing
|
26541
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
45.56%
|
|
|---|---|
|
Untested elements
|
191
|
|
Tested elements
|
164
|
Accessibility
92.54%
|
|
|---|---|
|
Failed rules
|
3 critical
8 serious
2 moderate
2 minor
|
|
Failed elements
|
887
|
Tests for review
src/specs/InlineRunAllSpecs.cy.tsx • 1 failed test • app-ct
| Test | Artifacts | |
|---|---|---|
| > Correctly rendered for Inline Specs list > provides expected tooltip content |
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
|
|
issues/28527.cy.ts • 1 flaky test • 5x-driver-chrome:beta
| Test | Artifacts | |
|---|---|---|
| issue 28527 > fails and then retries and verifies about:blank is not displayed |
Test Replay
Screenshots
|
|
issues/573.cy.js • 1 flaky test • 5x-driver-chrome:beta
| Test | Artifacts | |
|---|---|---|
| basic auth > can visit with username/pw in url |
Test Replay
|
|
e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| ... > throws when foo cannot resolve |
Test Replay
|
|
e2e/origin/config_env.cy.ts • 1 flaky test • 5x-driver-chrome
| Test | Artifacts | |
|---|---|---|
| cy.origin- Cypress.config() > serializable > overwrites different values in secondary if one exists in the primary |
Test Replay
|
|
The first 5 flaky specs are shown, see all 7 specs in Cypress Cloud.
|
There are some diffs in the error display after my changes. Looking into this. |
|
There's a Snyk failure that began 16 hours ago that seems unrelated. https://github.com/cypress-io/cypress/actions/workflows/snyk_sca_scan.yaml |
| @@ -1,5 +1,6 @@ | |||
| import FileMatch from './FileMatch.vue' | |||
| import { ref } from 'vue' | |||
| // tslint:disable-next-line: no-implicit-dependencies - unsure how to handle these | |||
There was a problem hiding this comment.
It looks like @cy/i18n is aliased in vite and ts configs as a relative inter-package import. @cy/components and @cy/gql-components are aliased in the same way ... we could probably use exports in @packages/frontend-shared to be able to import them correctly (e.g., import { defaultMessages } from '@packages/frontend-shared/i18n'). exports has fairly severe implications for encapsulation, though, and would probably be best left to a separate PR.
There was a problem hiding this comment.
@cacieprins Yeah, it's a weird one. I didn't want to touch it in this PR for sure.
| @@ -16,17 +16,23 @@ | |||
| "lint": "eslint --ext .js,.jsx,.ts,.tsx,.json, .", | |||
| "start": "echo 'run yarn dev from the root' && exit 1", | |||
| "test": "yarn cypress:run:ct && yarn types", | |||
| "tslint": "tslint --config ../ts/tslint.json --project .", | |||
| "tslint": "tslint --config ../ts/tslint.json --project . --exclude ../graphql/src/gen/nxs.gen.ts", | |||
There was a problem hiding this comment.
Why does this need to be excluded? with no implicit dependencies, we shouldn't be importing a sibling package's resource relatively, and tslint shouldn't lint dependencies (unless this is a defect in tslint)
There was a problem hiding this comment.
@cacieprins This is a great question that I don't have an answer to! I tried ignoring this file in SO many ways - and it is excluded in the graphql package itself, but tslint was still searching these files from a couple of other packages and erroring (because they're depended on it there, maybe something with circular deps??). I don't think this should be necessary but it is..
| @@ -5,6 +5,7 @@ import { cors } from '@packages/network' | |||
| import { urlOriginsMatch, urlSameSiteMatch } from '@packages/network/lib/cors' | |||
| import { SerializableAutomationCookie, Cookie, CookieJar, toughCookieToAutomationCookie } from '@packages/server/lib/util/cookies' | |||
| import type { RequestCredentialLevel } from '../../types' | |||
| // tslint:disable-next-line: no-implicit-dependencies - wants us to explicitly define cypress dep | |||
There was a problem hiding this comment.
This is exposing a code smell - the typedefs in cli should be imported from application code, rather than application code importing them from cli. Good to table for a future improvement.
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
We do not want implicit dependencies referenced in the monorepo. This creates really unexpected results with nx and our deps sometimes, especially when updating a dependency that you don't realize is actually being depended on in a package. A few of the dependencies we were including were never installed, but just sub-dependencies that happened to be in our yarn.lock file.
yarn why {dep}to get some idea of the version I thought the package was likely inheriting and specified that version.electrondep - I believe we had a problem with installing this on multiple packages before and the size exploded, so I don't know what we want to do here.cypressdep - don't think we want to explicitly install cypress on each package, so not sure how we want to handle that.@cy/i18n- this is being defined in some package's tsconfigpathsas a shortcute forpackages/frontend-shared/src/locales/i18n. It fails the check~icons/is declared as a module inpackages/app/vue-shims.d.ts. It fails the check.packages/graphql/src/gen/nxs.gen.tsfiles that are autogenerated. I had to ignore it in several places - probably because of some circular deps?Comprehensive list of errors fixed
Steps to test
Run tslint
Tests should pass
How has the user experience changed?
This shouldn't affect anything except for my sanity updating deps in the future.
PR Tasks
cypress-documentation?type definitions?