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: adding module support for invoking cypress.config.mjs, .cjs, .ts #20554

Merged
merged 23 commits into from
Mar 14, 2022

Conversation

JessicaSachs
Copy link
Contributor

@JessicaSachs JessicaSachs commented Mar 10, 2022

Summary

Adds support for Cypress Config files to work with modules. Uses @egoist 's bundle-require which was created for loading library config files like vite.config.ts

We now support the following config file types:

  1. cypress.config.mjs
  2. cypress.config.cjs
  3. cypress.config.js with "type": "module"
  4. cypress.config.ts with "type": "module"

We now support ESM inside of plugins hooks (setupNodeEvents + devServer)

Closes:

Changes made

  1. Adding module support + exports to the cypress node api (index.mjs, cli/package.json)
  2. Searching for additional file types (*.mjs, *.cjs) when resolving cypress.config
  3. Using bundle-require to execute Cypress Config
  4. Writing a ton of system tests for different package types

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 10, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 10, 2022



Test summary

17787 0 217 0Flakiness 1


Run details

Project cypress
Status Passed
Commit 9832322
Started Mar 14, 2022 3:00 AM
Ended Mar 14, 2022 3:12 AM
Duration 12:21 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/xhr.cy.js Flakiness
1 ... > no status when request isnt forced 404

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

@JessicaSachs JessicaSachs marked this pull request as ready for review March 10, 2022 04:43
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.

Nice job getting this through so quickly. I left some comments, most important one is adding the watcher to a cypress.config.mjs if it exists.

@elevatebart
Copy link
Contributor

Did you try it with UNIFY-1135 ?

@jennifer-shehane jennifer-shehane removed their request for review March 10, 2022 18:48
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.

Some comments!

@lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 self-requested a review March 11, 2022 04:31
lmiller1990
lmiller1990 previously approved these changes Mar 11, 2022
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.

all my blocking comments are ✔️ so seems fine - added 1 more question. Can we please assign someone from E2E to review this too?

"import": "./index.mjs",
"require": "./index.js"
},
"./package.json": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://nodejs.org/api/packages.html#package-entry-points allows package.json to be accessible. See "Package Entry Points"

tgriesser
tgriesser previously approved these changes Mar 11, 2022
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.

Thank you for adding this!

Comment on lines 111 to 113
if (err.stack.includes(`Cannot find package 'esbuild'`)) {
debug(`User doesn't have esbuild. Going to use native node imports.`)

Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this to work? Should we just throw an error that's like

"Hey, Cypress depends on esbuild for ESM support, please install it and try again"

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have it as a "real" dependency and only use it if typescript is detected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works fine for all TS projects where type != module. We don't need to fail -- we can just fall back.

if (legacyPlugins && typeof legacyPlugins.default === 'function') {
legacyPlugins = legacyPlugins.default
}
// Config file loading of modules is tested within
Copy link
Contributor

Choose a reason for hiding this comment

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

This got butchered during the rebase, I put all the "legacy config" stuff at the bottom.

@lmiller1990 lmiller1990 self-requested a review March 14, 2022 04:13
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.

Looks good. I merged in 10.0-release and resolved the conflicts.

@tgriesser tgriesser self-requested a review March 14, 2022 11:26
Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Works great, nice tests too 👍

@tgriesser tgriesser merged commit c2cc967 into 10.0-release Mar 14, 2022
@tgriesser tgriesser deleted the fix/typescript-module-config branch March 14, 2022 14:46
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.

5 participants