-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Fix issues with Cypress.require() and TypeScript #25931
fix: Fix issues with Cypress.require() and TypeScript #25931
Conversation
…s-require-typescript
…ypress-io/cypress into issue-25885-cypress-require-typescript
…s-require-typescript
…s-require-typescript
27 flaky tests on run #44611 ↗︎
Details:
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/navigation.cy.ts • 1 flaky test • 5x-driver-firefox
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox
create-from-component.cy.ts • 1 flaky test • app-e2e
The first 5 flaky specs are shown, see all 17 specs in Cypress Cloud. This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
@@ -0,0 +1,67 @@ | |||
const md5 = require('md5') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this file into the child process. The main reason for the content changes are converting it from typescript to vanilla JS, since we currently don't have the processing set up for TS within the child process.
@@ -14,6 +14,7 @@ const resolve = require('../../util/resolve') | |||
const browserLaunch = require('./browser_launch') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes in here was just moving a couple methods around so that the order made a little more sense (to me at least) when I was updating the tests for this. The only thing I added were the lines referencing _process:cross:origin:callback
.
@@ -1,433 +1,278 @@ | |||
require('../../../spec_helper') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these tests had been skipped for over a year, so I unskipped them, fixed the existing ones, and added new ones for my changes to run_plugins.js
. It doesn't have 100% test coverage of run_plugins.js
(I didn't add tests for any functionality I didn't add or change), but it's a step in the right direction.
@@ -1 +0,0 @@ | |||
!node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This origin-dependencies
project is no longer used by any system tests.
const typescriptPath = resolve.typescript(projectRoot) | ||
const webpackOptions = getFullWebpackOptions(file, typescriptPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the root of the fix. Instead of resolving typescript with our own, we get the path to it the same way we do for regular file preprocessing by finding the user's installed typescript.
@@ -7,6 +7,10 @@ _Released 03/14/2023 (PENDING)_ | |||
|
|||
- It is now possible to control the number of connection attempts to the browser using the CYPRESS_CONNECT_RETRY_THRESHOLD Environment Variable. Learn more [here](https://docs.cypress.io/guides/references/advanced-installation#Environment-variables). Addressed in [#25848](https://github.com/cypress-io/cypress/pull/25848). | |||
|
|||
**Bugfixes:** | |||
|
|||
- Fixed an issue where using `Cypress.require()` would throw the error `Cannot find module 'typescript'`. Fixes [#25885](https://github.com/cypress-io/cypress/issues/25885). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fixing a regression or did it never work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked with regular require
, but hasn't worked with Cypress.require()
since it was re-released.
|
||
options.webpackOptions = getDefaultWebpackOptions() | ||
|
||
addTypeScriptConfig({ filePath }, options) | ||
if (options.typescript) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the variable we want to check is typescript.options.typescript? Is there a better name for the top variable? Or could we get options directly passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't quite typescript.options.typescript, but taking another look at this, I realized it had grown a little convoluted so I refactored it a bit in fa10505
…ypress-io/cypress into issue-25885-cypress-require-typescript
…s-require-typescript
Additional details
The root of the fix is two-fold:
typescript
package instead of our own, which actually doesn't exist in the production build of Cypress. This is also the same way it works when preprocessing the spec file itself.Steps to test
Set up a test project with a TypeScript spec file and use
Cypress.require()
withincy.origin()
:How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?