fix: use posix path for ts-node loader #22550

lmiller1990 · 2022-06-28T06:40:36Z

User facing changelog

Correctly escape ts-node require when running on windows.

Additional details

Fixes a bug where TypeScript users with spaces in their path could not launch Cypress. Introduced in abd986a#diff-b0bd90502ad464c0d541a0e47822d15fec9ba5434dc4225d2993305e1ec7d30aR19. require.resolve returns a path which we pass to NODE_OPTIONS in the form of --require <path> which errors if there is white space in the path. The fix is to change the path to use posix separators.

Steps to test

go on windows
put cypress in a directory with white space
yarn dev wasn't working - now it is
you should also be able to load a project that's got white space in the path
you also should be able to migrate a project w/ white space - this might have explained some migration issues, too, for anyone migrating after this regression.

How has the user experience changed?

PR Tasks

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?
Have API changes been updated in the type definitions?

* fix: distribute files to machines for external contributors. * fix path * fix * fix glob * fix * fix glob pattern spec->cy. * fix * echo things. * test * use cd. * fix component tests. * test * test * fix * refactor * test distribut-step fix error fix fix test TEST * Revert "test distribut-step" This reverts commit 15c3606. * Revert "refactor" This reverts commit 21a8ad9. * reduce flake by increasing viewport height Co-authored-by: Ryan Manuel <ryanm@cypress.io> Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

…x-white-spaces

cypress-bot · 2022-06-28T06:40:38Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

lmiller1990 · 2022-06-28T08:11:35Z

packages/data-context/src/actions/MigrationActions.ts

@@ -103,7 +103,7 @@ export async function processConfigViaLegacyPlugins (projectRoot: string, legacy
    // this matches the 9.x behavior, which is what we want for
    // processing legacy pluginsFile (we never supported `"type": "module") in 9.x.
    if (hasTypeScriptInstalled(projectRoot)) {
-      const tsNodeLoader = `--require ${tsNode}`
+      const tsNodeLoader = `--require "${tsNode}"`


Before: --require C:\project with spaces\node_modules\ts-node\dist\ts-node.js. Broken
Now: `--require "C:/project with spaces/node_modules/ts-node/dist/ts-node.js"

Needs posix and quotes. Would be great if someone can actually test on their machine (I did, and I added two tests with projects containing spaces, but good to clarify).

cypress · 2022-06-28T09:05:12Z

Test summary

37681 • 0 • 456 • 0 • 10

Run details


Project	cypress
Status	Passed
Commit	`19280b2`
Started	Jun 28, 2022 8:47 AM
Ended	Jun 28, 2022 9:05 AM
Duration	17:58 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	actions/click.cy.js	1
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	xhr.cy.js	4
1	... > logs request + response headers
2	... > logs request + response headers
3	... > logs Method, Status, URL, and XHR
4	... > logs response
This comment includes only the first 5 flaky tests. See all 10 flaky tests in the Cypress Dashboard.

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

emilyrohrbough · 2022-06-28T12:48:14Z

packages/data-context/src/actions/MigrationActions.ts


 const debug = debugLib('cypress:data-context:MigrationActions')

-const tsNode = require.resolve('@packages/server/lib/plugins/child/register_ts_node')
+const tsNode = toPosix(require.resolve('@packages/server/lib/plugins/child/register_ts_node'))


Is this helper necessary? Can we use path.joint out of the box?

Suggested change

const tsNode = toPosix(require.resolve('@packages/server/lib/plugins/child/register_ts_node'))

const tsNode = require.resolve(path.join('@packages', 'server', 'lib','plugins','child', 'register_ts_node'))

@emilyrohrbough I believe the intent here is to properly escape the actual resolved path that's returned from require.resolve, which path.join wouldn't help with

Yes, the helper is the fix. We need to posix-ify the path, even on windows.

emilyrohrbough · 2022-06-28T12:48:47Z

packages/data-context/src/data/ProjectConfigIpc.ts

@@ -17,7 +17,7 @@ const debug = debugLib(`cypress:lifecycle:ProjectConfigIpc`)
 const CHILD_PROCESS_FILE_PATH = require.resolve('@packages/server/lib/plugins/child/require_async_child')


Lets fix CHILD_PROCESS_FILE_PATH one as well.

I wonder if this is actually a problem? Eg - I added regression tests, none of them fail. Kind of hesitant to add this helper unless it's actually necessary.

We use it like this:

const proc = fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)

I think fork must be smart enough to escape the path on its own.

Edit: I see https://docs.cypress.io/guides/references/advanced-installation#Binary-cache

Windows: /AppData/Local/Cypress/Cache

Since Cypress is always in a directory w/o a space, this has never been a problem. If it's never been a problem, I am kind of hesitant to add this change right now before shipping (without another round of manual testing and a way to reproduce the error it's meant to be fixing.

sainthkh and others added 4 commits June 28, 2022 15:37

fix: add baseUrl to TestConfigOverrides (#22445)

6c15a7b

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

handle white space when registering ts-node using --require

f914082

Merge remote-tracking branch 'origin/develop' into lmiller/windows-fi…

c246e37

…x-white-spaces

lmiller1990 force-pushed the lmiller/windows-fix-white-spaces branch from 8cfd2ee to c246e37 Compare June 28, 2022 06:42

update test project

ffe3fd0

This was referenced Jun 28, 2022

Cypress migration tool doesn't appear + stack trace in the console #22544

Closed

Cypress doesn't load or open its window #22297

Closed

lmiller1990 marked this pull request as ready for review June 28, 2022 08:08

lmiller1990 requested review from a team and tgriesser as code owners June 28, 2022 08:08

lmiller1990 requested review from mschile, astone123, mjhenkes, tbiethman, ZachJW34 and a team and removed request for a team, tgriesser and mschile June 28, 2022 08:08

lmiller1990 commented Jun 28, 2022

View reviewed changes

move config

19280b2

emilyrohrbough reviewed Jun 28, 2022

View reviewed changes

mjhenkes approved these changes Jun 28, 2022

View reviewed changes

tgriesser self-requested a review June 28, 2022 13:55

tgriesser approved these changes Jun 28, 2022

View reviewed changes

mjhenkes merged commit c0ea9bd into develop Jun 28, 2022

mjhenkes deleted the lmiller/windows-fix-white-spaces branch June 28, 2022 14:00

lmiller1990 mentioned this pull request Jun 28, 2022

Cypress hangs #22562

Closed

willfish mentioned this pull request Jun 30, 2022

Cypress upgrade to 10.3.0 trade-tariff/trade-tariff-testing#802

Merged

4 tasks

lmiller1990 mentioned this pull request Dec 19, 2022

Cypress is running really slow after upgrading to v10 #22868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use posix path for ts-node loader #22550

fix: use posix path for ts-node loader #22550

lmiller1990 commented Jun 28, 2022 •

edited

cypress-bot bot commented Jun 28, 2022

lmiller1990 Jun 28, 2022

cypress bot commented Jun 28, 2022

emilyrohrbough Jun 28, 2022

tgriesser Jun 28, 2022

lmiller1990 Jun 28, 2022

emilyrohrbough Jun 28, 2022 •

edited

lmiller1990 Jun 28, 2022

lmiller1990 Jun 28, 2022

	const tsNode = toPosix(require.resolve('@packages/server/lib/plugins/child/register_ts_node'))
	const tsNode = require.resolve(path.join('@packages', 'server', 'lib','plugins','child', 'register_ts_node'))

		@@ -17,7 +17,7 @@ const debug = debugLib(`cypress:lifecycle:ProjectConfigIpc`)
		const CHILD_PROCESS_FILE_PATH = require.resolve('@packages/server/lib/plugins/child/require_async_child')

fix: use posix path for ts-node loader #22550

fix: use posix path for ts-node loader #22550

Conversation

lmiller1990 commented Jun 28, 2022 • edited

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jun 28, 2022

lmiller1990 Jun 28, 2022

Choose a reason for hiding this comment

cypress bot commented Jun 28, 2022

Test summary

Run details

Flakiness

emilyrohrbough Jun 28, 2022

Choose a reason for hiding this comment

tgriesser Jun 28, 2022

Choose a reason for hiding this comment

lmiller1990 Jun 28, 2022

Choose a reason for hiding this comment

emilyrohrbough Jun 28, 2022 • edited

Choose a reason for hiding this comment

lmiller1990 Jun 28, 2022

Choose a reason for hiding this comment

lmiller1990 Jun 28, 2022

Choose a reason for hiding this comment

lmiller1990 commented Jun 28, 2022 •

edited

emilyrohrbough Jun 28, 2022 •

edited