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: use posix path for ts-node loader #22550

Merged
merged 6 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/data-context/src/actions/MigrationActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ import {
} from '../sources/migration'
import { makeCoreData } from '../data'
import { LegacyPluginsIpc } from '../data/LegacyPluginsIpc'
import { hasTypeScriptInstalled } from '../util'
import { hasTypeScriptInstalled, toPosix } from '../util'

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'))
Copy link
Member

Choose a reason for hiding this comment

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

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'))

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


export function getConfigWithDefaults (legacyConfig: any) {
const newConfig = _.cloneDeep(legacyConfig)
Expand Down Expand Up @@ -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}"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if (!childOptions.env) {
childOptions.env = {}
Expand Down
13 changes: 7 additions & 6 deletions packages/data-context/src/data/ProjectConfigIpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import fs from 'fs-extra'
import path from 'path'
import inspector from 'inspector'
import debugLib from 'debug'
import { autoBindDebug, hasTypeScriptInstalled } from '../util'
import { autoBindDebug, hasTypeScriptInstalled, toPosix } from '../util'
import _ from 'lodash'
import { pathToFileURL } from 'url'

Expand All @@ -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')
Copy link
Member

@emilyrohrbough emilyrohrbough Jun 28, 2022

Choose a reason for hiding this comment

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

Lets fix CHILD_PROCESS_FILE_PATH one as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


const tsNodeEsm = pathToFileURL(require.resolve('ts-node/esm/transpile-only')).href
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'))

export type IpcHandler = (ipc: ProjectConfigIpc) => void

Expand Down Expand Up @@ -128,6 +128,7 @@ export class ProjectConfigIpc extends EventEmitter {
let resolved = false

this._childProcess.on('error', (err) => {
debug('unhandled error in child process %s', err)
this.handleChildProcessError(err, this, resolved, reject)
reject(err)
})
Expand All @@ -139,6 +140,7 @@ export class ProjectConfigIpc extends EventEmitter {
* but it's not.
*/
this.on('childProcess:unhandledError', (err) => {
debug('unhandled error in child process %s', err)
this.handleChildProcessError(err, this, resolved, reject)
reject(err)
})
Expand All @@ -150,6 +152,7 @@ export class ProjectConfigIpc extends EventEmitter {
})

this.once('loadConfig:error', (err) => {
debug('error loading config %s', err)
this.killChildProcess()
reject(err)
})
Expand Down Expand Up @@ -289,7 +292,7 @@ export class ProjectConfigIpc extends EventEmitter {
// We do NOT use the `--loader` flag because we have some additional
// custom logic for ts-node when used with CommonJS that needs to be evaluated
// so we need to load and evaluate the hook first using the `--require` module API.
const tsNodeLoader = `--require ${tsNode}`
const tsNodeLoader = `--require "${tsNode}"`

if (childOptions.env.NODE_OPTIONS) {
childOptions.env.NODE_OPTIONS += ` ${tsNodeLoader}`
Expand All @@ -302,9 +305,7 @@ export class ProjectConfigIpc extends EventEmitter {
// TODO: Consider using userland `esbuild` with Node's --loader API to handle ESM.
}

const proc = fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)

return proc
return fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)
}

private handleChildProcessError (err: any, ipc: ProjectConfigIpc, resolved: boolean, reject: (reason?: any) => void) {
Expand Down
4 changes: 2 additions & 2 deletions packages/launchpad/cypress/e2e/migration.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1546,8 +1546,8 @@ describe('Migration', { viewportWidth: 1200, retries: { openMode: 0, runMode: 2
})

describe('Migrate custom config files', () => {
it('completes journey for migration-custom-config-file-root-level', () => {
startMigrationFor('migration-custom-config-file-root-level', ['--config-file', 'customConfig.json'])
it('completes journey for migration-custom-config-file-root-level spaces', () => {
startMigrationFor('migration-custom-config-file-root-level spaces', ['--config-file', 'customConfig.json'])

// defaults, rename all the things
// can rename integration->e2e
Expand Down
7 changes: 7 additions & 0 deletions packages/launchpad/cypress/e2e/open-mode.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,4 +242,11 @@ describe('Launchpad: Open Mode', () => {
cy.get('body').should('not.contain.text', 'Your project does not contain a default supportFile.')
cy.get('h1').should('contain', 'Choose a Browser')
})

it('opens project with spaces in path', () => {
cy.scaffoldProject('simple with spaces')
cy.openProject('simple with spaces', ['--e2e'])
cy.visitLaunchpad()
cy.get('h1').should('contain', 'Choose a Browser')
})
})
11 changes: 10 additions & 1 deletion scripts/run-webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,13 @@ if (process.versions && semver.satisfies(process.versions.node, '>=17.0.0') && s
NODE_OPTIONS = `${NODE_OPTIONS} --openssl-legacy-provider`
}

cp.execSync(`node ${webpackCli} ${process.argv.slice(2).join(' ')}`, { stdio: 'inherit', env: { ...process.env, NODE_OPTIONS } })
function buildCommand () {
const file = process.argv.slice(2).join(' ') ?? ''
let program = `node "${webpackCli}"`

return file ? `${program } "${file}"` : program
}

const program = buildCommand()

cp.execSync(program, { stdio: 'inherit', env: { ...process.env, NODE_OPTIONS } })
8 changes: 8 additions & 0 deletions system-tests/projects/simple with spaces/cypress.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module.exports = {
e2e: {
supportFile: false,
setupNodeEvents (on, config) {
// implement node event listeners here
},
},
}
3 changes: 3 additions & 0 deletions system-tests/projects/simple with spaces/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"projectFixtureDirectory": "simple_passing"
}