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

fix: support loading config files for projects with type module, without esbuild #21574

Merged
merged 24 commits into from
May 31, 2022

Conversation

JessicaSachs
Copy link
Contributor

@JessicaSachs JessicaSachs commented May 19, 2022

@flotwig or @lmiller1990 I could use a hand in figuring out why the existing test suite here does not fail.

Do we always have esbuild in the require callstack? I thought we sandboxed the node_modules well enough in the temp dir.

User facing changelog

We now support Cypress Config files for project that have "type": "module" in their package.json, but do not have a dependency on esbuild.

@JessicaSachs JessicaSachs requested a review from a team as a code owner May 19, 2022 22:05
@JessicaSachs JessicaSachs requested review from jennifer-shehane and removed request for a team May 19, 2022 22:05
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 19, 2022

Thanks for taking the time to open a PR!

@tgriesser tgriesser mentioned this pull request May 19, 2022
@JessicaSachs JessicaSachs dismissed tgriesser’s stale review May 20, 2022 13:52

Not changing code flow.

@jennifer-shehane jennifer-shehane removed their request for review May 20, 2022 18:30
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

Would like to see some tests for this, either fixing the existing tests or a new integration test. This error message comparison seems prone to regressions.

@JessicaSachs
Copy link
Contributor Author

There was a test already written, but it wasn't failing correctly due to differences between prod and test.

When running system-tests, require.resolve uses the node_modules closest to cypress/packages/server/lib/plugins/child/run_require_async_child.js to figure out which esbuild to use -- so, when it came time to test this code path, esbuild was always present and we were never hitting the codepath that the test:

https://github.com/cypress-io/cypress/blob/c2cc967227c6359203e3c1bdb99fe54e8fef996f/system-tests/projects/config-cjs-and-esm/config-with-js-module/package.json

was designed to cover.

By adding paths: [process.cwd()] into the require.resolve statement, the test actually tests something and will now correctly fail.

@JessicaSachs JessicaSachs requested a review from flotwig May 23, 2022 18:16
@@ -111,14 +111,14 @@ function run (ipc, file, projectRoot) {
try {
debug('Trying to use esbuild to run their config file.')
// We prefer doing this because it supports TypeScript files
require.resolve('esbuild')
require.resolve('esbuild', { paths: [process.cwd()] })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The absence of this line was actually a bug and it was only working on my machine because of some GLOBAL transitive deps with esbuild.

To debug this, you can add

require.resolve.paths('esbuild')

to a production-built binary installed in a real project. We should really consider moving some of these low-dependency or no-dependency tests OUTSIDE of the system-tests and into the binary-system-tests phase.

@cypress
Copy link

cypress bot commented May 24, 2022



Test summary

58 0 0 0Flakiness 1


Run details

Project cypress
Status Passed
Commit 71cbb69
Started May 31, 2022 6:20 PM
Ended May 31, 2022 6:34 PM
Duration 13:44 💡
OS Linux Debian - 10.11
Browser Chrome 100

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/next.cy.ts Flakiness
1 Working with next-12.1.6 > should live-reload on src changes

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
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.

Looks like there's a test failing now just FYI. Think it might just be a matter of adding esbuild to config-with-ts-module-error system test or something

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@JessicaSachs Update the base branch to be against develop before merging.

@JessicaSachs JessicaSachs changed the base branch from 10.0-release to develop May 26, 2022 19:16
@JessicaSachs JessicaSachs requested a review from a team as a code owner May 26, 2022 19:31
@jennifer-shehane jennifer-shehane dismissed their stale review May 27, 2022 17:49

Dismissing my review as addressed

@JessicaSachs JessicaSachs requested a review from tgriesser May 27, 2022 18:47
@JessicaSachs JessicaSachs dismissed tgriesser’s stale review May 27, 2022 18:51

Trying to kick circle off..?

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.

Updated the tests to capture the fact that without esbuild, .ts files do not work. Logged #21939 to track updates to support that.

@tgriesser tgriesser merged commit 996823c into develop May 31, 2022
@tgriesser tgriesser deleted the fix/module-projects-without-esbuild branch May 31, 2022 18:25
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.

6 participants