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

feat(angular): angular mount #22858

Merged
merged 15 commits into from
Jul 26, 2022
Merged

feat(angular): angular mount #22858

merged 15 commits into from
Jul 26, 2022

Conversation

jordanpowell88
Copy link
Collaborator

@jordanpowell88 jordanpowell88 commented Jul 19, 2022

User facing changelog

Adds @cypress/angular mount

Additional details

Steps to test

We can use system tests from #22314 to validate this work

How has the user experience changed?

Nothing changes from a user experience as this is a new feature

PR Tasks

  • [ na] Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • [ na] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 19, 2022

Thanks for taking the time to open a PR!

@jordanpowell88 jordanpowell88 changed the title Angular mount feat(angular): angular mount Jul 19, 2022
@jordanpowell88 jordanpowell88 marked this pull request as ready for review July 20, 2022 15:19
@jordanpowell88 jordanpowell88 requested review from a team as code owners July 20, 2022 15:19
@jordanpowell88 jordanpowell88 requested review from ryanthemanuel and removed request for a team July 20, 2022 15:19
@cypress
Copy link

cypress bot commented Jul 20, 2022



Test summary

4379 0 51 0Flakiness 1


Run details

Project cypress
Status Passed
Commit b54eac5
Started Jul 26, 2022 10:32 PM
Ended Jul 26, 2022 10:43 PM
Duration 11:15 💡
OS Linux Debian - 11.3
Browser Firefox 99

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/net_stubbing.cy.ts Flakiness
1 network stubbing > waiting and aliasing > should handle aborted requests

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Couple of small questions! Excited for this.

cli/angular/README.md Outdated Show resolved Hide resolved
component: Type<T>,
config: TestBedConfig<T> = {},
autoDetectChanges = true,
): Cypress.Chainable<MountResponse<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our other adapters (react, vue) both accept some additional args that let you inject some stylesheets. I wonder if this is something that would also be useful for angular?

This is implemented using @cypress/mount-utils.

React example:

import {
injectStylesBeforeElement,
StyleOptions,
getContainerEl,
ROOT_SELECTOR,
setupHooks,
} from '@cypress/mount-utils'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a behavior that you cannot do in Angular. You cannot just import a stylesheet at the component level. You have global stylesheets which will be brought in and added to the webpack compilation via the users angular.json file, and the component's stylesheet itself


return component
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we actually mount the component? React/Vue use a data-cy-root selector which they import https://github.com/cypress-io/cypress/blob/develop/npm/react/src/mount.ts#L8

I don't see how we go from component -> mount to DOM here. Am I missing it, or is that implemented in another PR?

https://github.com/cypress-io/cypress/pull/22858/files#diff-617e9faa6b88ce75b6ca3135c54ae7000965719cc2d6c9ae9813178d9da19170

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By default Angular mounts the component for you to the DOM using the angular.json build.options.index property. In THIS PR we change this value from what the user has to cypress/support/component-index.html. It is then in that file (that we will need to generate as part of the migration work) that angular will mount the component

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

My questions were answered 👍

Did not try locally yet but code is 💯

@lmiller1990
Copy link
Contributor

Seems linting and a few other steps are failing, might be good to get those green before any other reviews.

@@ -1791,14 +1791,6 @@ jobs:
- run:
name: Build
command: yarn workspace @cypress/angular build
- run:
Copy link
Contributor

Choose a reason for hiding this comment

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

No tests for npm/angular. To test this, we would need to install all of the webpack deps for Angular so all of the mounting tests are found in the system-tests/project-fixtures/angular/src/app/mount.cy.ts which are run in npm-webpack-dev-server e2e tests.

@@ -1,221 +1,154 @@
# @cypress/angular
Copy link
Contributor

Choose a reason for hiding this comment

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

Should clean this up to be a minimal README like the other mount packages since most of this information will be in the official docs.

"@cypress/mount-utils": "0.0.0-development",
"debug": "^4.3.2"
"prebuild": "rimraf dist",
"build": "tsc || echo 'built, with type errors'",
Copy link
Contributor

@ZachJW34 ZachJW34 Jul 25, 2022

Choose a reason for hiding this comment

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

Realized that rollup is overkill for this package. The versions of Angular we support expect esm packages so we only need to publish one entrypoint, so using tsc directly is sufficient.

@@ -0,0 +1,225 @@
import 'zone.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to move side-effects into a separate file, though this would increase the complexity of scaffolding during launchpad setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this illustrates the problem of trying to have no side effects - this import makes perfect sense here, it's required for Angular mounting, so it should be as near the mount code as possible.

An alternative would be scaffold it in component.ts, but then we increase the complexity of component.js for... not much benefit. imo. Also, is there a downside to having people import/bundle zone.js in E2E? (If they don't want to, they just shouldn't mix their E2E and CT support code).

Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be any issues for importing zone.js in e2e tests. I was harping on not having side effects a while ago but now I would rather have side-effects if it means for a cleaner support file and less complexity during project setup.

"./sinon-chai/*",
// Copied from net-stubbing and renamed to a declaration file. Since it's not originally
// a declaration file, we need to exclude it from linting.
"./net-stubbing.d.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed #22417 as a part of this work. Angular crawls all of the imported types and emits warnings due to this file not being included in the compilation. .d.ts files are fine and it's what this file should be anyway. I had to ignore this file in the tslint.json as dtslint complains. Since this file is originally a normal ts file, fixing the dtslint warnings causes ts warnings, so ignoring seemed best.

@jordanpowell88
Copy link
Collaborator Author

2 things I am noticing after doing some additional testing with the binary is that we don't have a good way to support the following 2 scenarios:

  • content project (ie: <ng-content></ng-content>
  • Outputs: (we need a way to spy on the emitted events)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Angular mount package node_modules/cypress/types/net-stubbing.ts should be a .d.ts file
4 participants