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: embedding mount into the cypress binary (real dependency) #20930

Merged
merged 16 commits into from
Apr 7, 2022

Conversation

JessicaSachs
Copy link
Contributor

@JessicaSachs JessicaSachs commented Apr 5, 2022

Approach

This PR essentially copies the built libraries from npm/vue, npm/react, and npm/mount-utils and keeps the cli/package.json's exports map up to date.

This avoids making changes to specific dev-servers or relying on specific bundler features like resolve.alias.

Resources

User facing changelog

Cypress users no longer have to install the Vue or React mounting libraries separately when mounting components. If you are using Vue 3 or React, you can remove those dependencies from your package.json entirely and replace the imports to the newly supported cypress/vue and cypress/react namespaces.

For Vue 2 users, a separate pull request will add support for importing mount from cypress/vue-2.

Before: Externally Install Mounting Libs

Mounting libraries are always explicitly imported from the Cypress npm organization namespace.

// React
import { mount } from '@cypress/react'

// Vue 3
import { mount } from '@cypress/vue' // v3.x, pinned in your package.json

// Vue 2
import { mount } from '@cypress/vue' // v2.x, pinned in your package.json

After: Bundling Mounting Libs

Mounting libraries are shipped with Cypress, and are exposed as subpaths form the Cypress namespace. This is done as part of the cli build process.

This API is supported via the cypress package's exports map as defined in the package.json.

// React
import { mount } from 'cypress/react'

// Vue 3
import { mount } from 'cypress/vue'

// Vue 2, still have to npm install the pinned version of @cypress/vue for now
import { mount } from '@cypress/vue'

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 5, 2022

Thanks for taking the time to open a PR!

@JessicaSachs JessicaSachs changed the title feat: embedding mount into the cypress binary feat: embedding mount into the cypress binary (real dependency) Apr 5, 2022
@cypress
Copy link

cypress bot commented Apr 5, 2022



Test summary

17812 0 217 0Flakiness 2


Run details

Project cypress
Status Passed
Commit ca18b0a
Started Apr 6, 2022 11:13 PM
Ended Apr 6, 2022 11:25 PM
Duration 11:55 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/xhr.cy.js Flakiness
1 ... > no status when request isnt forced 404
cypress/proxy-logging.cy.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

cli/.gitignore Outdated Show resolved Hide resolved
@JessicaSachs
Copy link
Contributor Author

It works!!!! Going to remove the circle changes.

@JessicaSachs JessicaSachs marked this pull request as ready for review April 6, 2022 23:07
@JessicaSachs JessicaSachs requested review from a team as code owners April 6, 2022 23:07
@JessicaSachs JessicaSachs requested review from jennifer-shehane and tgriesser and removed request for a team April 6, 2022 23:07
Copy link
Member

@tgriesser tgriesser left a comment

Choose a reason for hiding this comment

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

Nice! We can probably look into symlinking the dirs for local development in a follow up PR

"require": "./react/dist/cypress-react.cjs.js"
},
"./mount-utils": {
"require": "./mount-utils/dist/index.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

No import entry for mount-utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it wasn't built for ESM.

Because only cypress-react and cypress-vue consume it and they have an esm export it's not (currently) a problem.

We have tech debt to convert all of our npm/* packages to be esm-first and share the same rollup + typescript compilation settings.

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.

Code seems fine, just have a comment around how we can test this and the end goal (we will migrate to import ... from 'cypress/vue' internally? How do we QA and verify this works (no tests for it so far).

console.log('Writing to CLI package.json for', exportName)

fs.writeFileSync(path.join(cliPath, 'package.json'), output, 'utf-8')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The tiniest thing but this seems like pretty important logic to test - do we need some kind of basic test to ensure that the package.json updates correctly, and that import { mount } from 'cypress/vue is actually working? We don't have any coverage for this right now, from what I can see.

I wonder if we can update all internal code to use the deep import syntax in the future. Is this something we plan to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can test it by adding code here cypress-io/cypress-example-kitchensink#528

I'll do that. It'll break all other pipelines until this is merged, maybe I'll create a new branch for it (in kitchensink).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also wondering if we can convert a couple of the existing system tests to import { mount } from 'cypress/vue' rather than import { mount } from '@cypress/vue'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That probably only works in test-binary, where we have tests that run in a docker container against the compiled binary. But still worth looking into. Just an extremely simple 'can import' test there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah in general I feel uncomfortable that we don't have a lot of post-build infrastructure for testing the output of Circle CI's build scripts. The kitchen sink is the right place. I put a PR up above that will at least fail if those packages aren't declared in the exports map.

@@ -22,6 +22,10 @@ includeTypes.forEach((folder) => {
shell.cp('-R', source, 'build/types')
})

// TODO: Add a typescript or rollup build step
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a github issue for this we can link? Have been trying to do TODO: blah, https://github.com/blahblah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't right now.

@BlueWinds
Copy link
Contributor

Looks good to me. I'd love to see a binary system test to cover this, but if you want to hold off for a separate PR on that, I'll approve this one as-is.

@JessicaSachs JessicaSachs merged commit 3fe5f50 into 10.0-release Apr 7, 2022
@JessicaSachs JessicaSachs deleted the feat/embedding-mount-with-real-package branch April 7, 2022 00:12
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.

None yet

4 participants