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.

Sign up for GitHub

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

Jump to bottom

chore: Intregrate runner packages. #23028

Merged
merged 13 commits into from
Aug 11, 2022
Merged

chore: Intregrate runner packages. #23028

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
54b73b5
chore: Intregrate runner packages.
sainthkh Aug 1, 2022
e836b14
Remove unnecessary Studio react files.
sainthkh Aug 1, 2022
1c4e85e
Remove unnecessary gif
sainthkh Aug 1, 2022
73fdfb4
runner-shared to runner-ct.
sainthkh Aug 1, 2022
8fab511
fix path.
sainthkh Aug 1, 2022
a00e2aa
fix package.json
sainthkh Aug 1, 2022
e636e95
Remove scss files from runner-ct
sainthkh Aug 1, 2022
d1c7754
Remove runner-ct
sainthkh Aug 1, 2022
e1b887f
Remove runner-shared and runner-ct comments.
sainthkh Aug 2, 2022
166cc8a
Feedback
sainthkh Aug 3, 2022
73fc73c
Merge branch 'develop' into issue-21689
emilyrohrbough Aug 10, 2022
a4b4f7f
chore: reduce parallelism for reporter-componen-tests
lmiller1990 Aug 11, 2022
cccd76e
chore: reduce paralelleism
lmiller1990 Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@
/packages/rewriter/ @cypress-io/end-to-end
/packages/root/ @cypress-io/end-to-end
/packages/runner/ @cypress-io/end-to-end
/packages/runner-ct/ @cypress-io/component-testing
/packages/runner-shared/ @cypress-io/end-to-end
/packages/server/ @cypress-io/end-to-end
/packages/socket/ @cypress-io/end-to-end
/packages/static/ @cypress-io/end-to-end
Copy link
Contributor

@lmiller1990 lmiller1990 Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

FYI some context - we ideally should delete ALL of runner/runner-shared, and bundle the entire app with the Vite stack.

The reason we are stuck like this is explained a little here: #20663 (comment)

Basically some of the code in driver and possibly reporter are not really ESM compatible - webpack does something different to Vite to resolve the modules, which is why it works in webpack, but we cannot import to Vite.

Not sure if this is something you are looking to investigate more @sainthkh but if you do please let me know, I'd like to follow along/help out - I'd really like to get away from this "inject webpack bundle" mess at some point, but it's quite hard. Likely making driver 100% ESM (including dependencies) would be a good first step.

Any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that we have 4 things to do to delete the runner pacakge.

  • Migrate dom. -> I think driver/src/dom might be a good place or move it to app directly.
  • Reimplement Studio -> Cypress Studio Reimplementation Spike #22870
  • ESM-ify driver -> I agree it's the biggest problem.
  • Let reporter compile its own styles. -> Move runner/webpack.config.ts to reporter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dom -> are you going do to this here?
Studio -> I am looking into this now
ESMify driver -> agree, hard
Reporter styles -> do this separately I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read the dom code and it seems that it's better to be a separate PR.

It currently renders highlight by

  1. Creating the container node with jQuery.
  2. Rendering the Highlight DOM with React.

I can simply migrate dom to app and use unified-runner. But I don't think it's not our goal. It's just hiding and postponing.

To get migrating dom done, I need to do these things:

  • Remove JQuery from dom and dimensions.
  • Move the current JQuery version of dimensions to driver/src/dom because it's used in driver/src/dom/blackout.
  • Migrate Highlight from react to vue with our Tooltip component.
  • Migrate dom functions to app/runner/aut-iframe. Leave studio-related functions.
  • Clean up unified-runner dom and CypressJQuery. They're only used inside aut-iframe. I guess they can be removed safely from unified-runner.

I think it's a big change and outside the scope of this PR.

As for reporter styles, it's safe to do it after ESM-ifying driver. We're trying to remove webpack. I don't think we don't need to create one more webpack.config.ts now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened the new PR #23187 to migrate dom.js. It's currently empty because I'm now reading and planning.

Expand Down
3 changes: 0 additions & 3 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@ system-tests/lib/fixtureDirs.ts
# from npm/webpack-dev-server
/npm/webpack-dev-server/cypress/videos

# from runner-ct
/packages/runner-ct/cypress/screenshots

# from errors
/packages/errors/__snapshot-images__
/packages/errors/__snapshot-md__
Expand Down
7 changes: 0 additions & 7 deletions .vscode/terminals.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,6 @@
"cwd": "[cwd]/packages/runner",
"command": "yarn watch"
},
{
"name": "packages/runner-ct watch",
"focus": true,
"onlySingle": true,
"cwd": "[cwd]/packages/runner-ct",
"command": "yarn watch"
},
{
"name": "packages/driver cypress open",
"focus": true,
Expand Down
2 changes: 0 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,6 @@ Here is a list of the core packages in this repository with a short description,
| [reporter](./packages/reporter) | `@packages/reporter` | The reporter shows the running results of the tests (The Command Log UI). |
| [root](./packages/root) | `@packages/root` | Dummy package pointing at the root of the repository. |
| [runner](./packages/runner) | `@packages/runner` | The runner is the minimal "chrome" around the user's application under test. |
| [runner-ct](./packages/runner-ct) | `@packages/runner-ct` | The runner for component testing |
| [runner-shared](./packages/runner-shared) | `@packages/runner-shared` | The shared components between the `runner` and the `runner-ct` packages |
| [server](./packages/server) | `@packages/server` | The <3 of Cypress. This orchestrates everything. The backend node process. |
| [server-ct](./packages/server-ct) | `@packages/server-ct` | Some Component Testing specific overrides. Mostly extends functionality from `@packages/server` |
| [socket](./packages/socket) | `@packages/socket` | A wrapper around socket.io to provide common libraries. |
Expand Down
4 changes: 2 additions & 2 deletions packages/app/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ The terminology can get a bit confusing as Vue Router's `params` are not the que

## Using existing, Vite-incompatible modules

Some of our modules, like `@packages/reporter`, `@packages/driver` and `@packages/runner-shared` cannot be easily
Some of our modules, like `@packages/reporter`, `@packages/driver` and `@packages/runner` cannot be easily
used with Vite due to circular dependencies and modules that do not have compatible ESM builds.

To work around this, when consuming existing code, it is bundled with webpack and made available under the
`window.UnifiedRunner` namespace. It is injected via [`injectBundle`](./src/runner/injectBundle.ts).

To add more code to the bundle, add it in the bundle root, `@packages/runner-ct/src/main.tsx` and attach it to
To add more code to the bundle, add it in the bundle root, `@packages/runner/src/main.tsx` and attach it to
`window.UnifiedRunner`.

As a rule of thumb, avoid importing from the older, webpack based modules into this package. Instead, if you want to consume code from those older, webpack bundled modules, you should add them to the webpack root and consume them via `window.UnifiedRunner`. Ideally, update [`index.d.ts`](./index.d.ts) to add the types, as well.
Expand Down
4 changes: 2 additions & 2 deletions packages/app/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ declare global {

/**
* Any React components or general code needed from
* runner-shared, reporter or driver are also bundled with
* runner, reporter or driver are also bundled with
* webpack and made available via the window.UnifedRunner namespace.
*
* We cannot import the correct types, because this causes the linter and type
* checker to run on runner-shared and reporter, and it blows up.
* checker to run on runner and reporter, and it blows up.
*/
Reporter: any
shortcuts: {
Expand Down
2 changes: 1 addition & 1 deletion packages/app/src/runner/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* - reporter
* which are built with React and bundle with webpack.
*
* The entry point for the webpack bundle is `runner-ct/main.tsx`.
* The entry point for the webpack bundle is `runner/main.tsx`.
* Any time you need to consume some existing code, add it to the `window.UnifiedRunner`
* namespace there, and access it with `window.UnifiedRunner`.
*
Expand Down
1 change: 0 additions & 1 deletion packages/driver/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
"@packages/network": "0.0.0-development",
"@packages/rewriter": "0.0.0-development",
"@packages/runner": "0.0.0-development",
"@packages/runner-shared": "0.0.0-development",
"@packages/server": "0.0.0-development",
"@packages/socket": "0.0.0-development",
"@packages/ts": "0.0.0-development",
Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/dom/blackout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import $ from 'jquery'
import $dimensions from '@packages/runner-shared/src/dimensions'
import $dimensions from '@packages/runner/src/dom/dimensions'

const resetStyles = `
border: none !important;
Expand Down
2 changes: 1 addition & 1 deletion packages/resolve-dist/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'path'

let fs: typeof import('fs-extra')

export type RunnerPkg = 'app' | 'runner' | 'runner-ct'
export type RunnerPkg = 'app' | 'runner'

type FoldersWithDist = 'static' | 'driver' | RunnerPkg | 'launchpad'

Expand Down
137 changes: 0 additions & 137 deletions packages/runner-ct/.eslintrc.json
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion packages/runner-ct/.gitignore
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions packages/runner-ct/README.md
View file
Open in desktop

This file was deleted.

69 changes: 0 additions & 69 deletions packages/runner-ct/package.json
View file
Open in desktop

This file was deleted.

36 changes: 0 additions & 36 deletions packages/runner-ct/src/SpecList/components/SearchSpec.scss
View file
Open in desktop

This file was deleted.

Loading