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 ESM projects using TypeScript with ts-node/esm #22118

Merged
merged 16 commits into from
Jun 9, 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
20 changes: 20 additions & 0 deletions packages/data-context/src/actions/MigrationActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ import {
} from '../sources/migration'
import { makeCoreData } from '../data'
import { LegacyPluginsIpc } from '../data/LegacyPluginsIpc'
import { hasTypeScriptInstalled } from '../util'

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

export function getConfigWithDefaults (legacyConfig: any) {
const newConfig = _.cloneDeep(legacyConfig)
Expand Down Expand Up @@ -88,6 +91,23 @@ export async function processConfigViaLegacyPlugins (projectRoot: string, legacy
const configProcessArgs = ['--projectRoot', projectRoot, '--file', cwd]
const CHILD_PROCESS_FILE_PATH = require.resolve('@packages/server/lib/plugins/child/require_async_child')

// use ts-node if they've got typescript installed
// 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}`

if (!childOptions.env) {
childOptions.env = {}
}

if (childOptions.env.NODE_OPTIONS) {
childOptions.env.NODE_OPTIONS += ` ${tsNodeLoader}`
} else {
childOptions.env.NODE_OPTIONS = tsNodeLoader
}
}

const childProcess = fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)
const ipc = new LegacyPluginsIpc(childProcess)

Expand Down
59 changes: 58 additions & 1 deletion packages/data-context/src/data/ProjectConfigIpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,21 @@ import { CypressError, getError } from '@packages/errors'
import type { FullConfig, TestingType } from '@packages/types'
import { ChildProcess, fork, ForkOptions } from 'child_process'
import EventEmitter from 'events'
import fs from 'fs-extra'
import path from 'path'
import inspector from 'inspector'
import debugLib from 'debug'
import { autoBindDebug } from '../util'
import { autoBindDebug, hasTypeScriptInstalled } from '../util'
import _ from 'lodash'

const pkg = require('@packages/root')
const debug = debugLib(`cypress:lifecycle:ProjectConfigIpc`)

const CHILD_PROCESS_FILE_PATH = require.resolve('@packages/server/lib/plugins/child/require_async_child')

const tsNodeEsm = require.resolve('ts-node/esm/transpile-only')
const tsNode = require.resolve('@packages/server/lib/plugins/child/register_ts_node')

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

export interface SetupNodeEventsReply {
Expand Down Expand Up @@ -238,6 +242,59 @@ export class ProjectConfigIpc extends EventEmitter {

debug('fork child process %o', { CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions: _.omit(childOptions, 'env') })

let isProjectUsingESModules = false
Copy link
Member

Choose a reason for hiding this comment

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

With this solution should any code be removed? I'm not super familiar with this code, but it seems like we might have some dead references esbuild code if this block takes over that responsibility. You mention a couple of places in your explanation, can we clean any of those up?

Copy link
Contributor Author

@lmiller1990 lmiller1990 Jun 7, 2022

Choose a reason for hiding this comment

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

I do think we can clean up some of the esbuild code (it's not working properly; we could delete it all, if we decide not to go down the esbuild route).

I figured I'd leave it untouched and make the changes as minimal as possible here. Having read your comment, I'm thinking we delete it, and bring it back when re-visiting #22074.

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 tried removing the esbuild code path here: f1ed508. We can revisit how best to use esbuild, if users want to use it, in the near future.


try {
const pkgJson = fs.readJsonSync(path.join(this.projectRoot, 'package.json'))

isProjectUsingESModules = pkgJson.type === 'module'
} catch (e) {
// project does not have `package.json` or it was not found
// reasonable to assume not using es modules
}

if (!childOptions.env) {
childOptions.env = {}
}

// If they've got TypeScript installed, we can use
// ts-node for CommonJS
// ts-node/esm for ESM
if (hasTypeScriptInstalled(this.projectRoot)) {
if (isProjectUsingESModules) {
// Use the ts-node/esm loader so they can use TypeScript with `"type": "module".
// The loader API is experimental and will change.
// The same can be said for the other alternative, esbuild, so this is the
// best option that leverages the existing modules we bundle in the binary.
// @see ts-node esm loader https://typestrong.org/ts-node/docs/usage/#node-flags-and-other-tools
// @see Node.js Loader API https://nodejs.org/api/esm.html#customizing-esm-specifier-resolution-algorithm
const tsNodeEsmLoader = `--experimental-specifier-resolution=node --loader ${tsNodeEsm}`

if (childOptions.env.NODE_OPTIONS) {
childOptions.env.NODE_OPTIONS += ` ${tsNodeEsmLoader}`
} else {
childOptions.env.NODE_OPTIONS = tsNodeEsmLoader
}
} else {
// Not using ES Modules (via "type": "module"),
// so we just register the standard ts-node module
// to handle TypeScript that is compiled to CommonJS.
// 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}`

if (childOptions.env.NODE_OPTIONS) {
childOptions.env.NODE_OPTIONS += ` ${tsNodeLoader}`
} else {
childOptions.env.NODE_OPTIONS = tsNodeLoader
}
}
} else {
// Just use Node's built-in ESM support.
// TODO: Consider using userland `esbuild` with Node's --loader API to handle ESM.
}

const proc = fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)

return proc
Expand Down
15 changes: 11 additions & 4 deletions packages/data-context/src/data/ProjectConfigManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,17 @@ export class ProjectConfigManager {
this._eventsIpc.cleanupIpc()
}

this._eventsIpc = new ProjectConfigIpc(this.options.ctx.nodePath, this.options.projectRoot, this.configFilePath, this.options.configFile, (cypressError: CypressError, title?: string | undefined) => {
this._state = 'errored'
this.options.ctx.onError(cypressError, title)
}, this.options.ctx.onWarning)
this._eventsIpc = new ProjectConfigIpc(
this.options.ctx.nodePath,
this.options.projectRoot,
this.configFilePath,
this.options.configFile,
(cypressError: CypressError, title?: string | undefined) => {
this._state = 'errored'
this.options.ctx.onError(cypressError, title)
},
this.options.ctx.onWarning,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code is the same, I just spaced it out so it's easier to read.


this._loadConfigPromise = this._eventsIpc.loadConfig()
}
Expand Down
9 changes: 9 additions & 0 deletions packages/data-context/src/util/hasTypescript.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export function hasTypeScriptInstalled (projectRoot: string) {
try {
require.resolve('typescript', { paths: [projectRoot] })

return true
} catch (e) {
return false
}
}
1 change: 1 addition & 0 deletions packages/data-context/src/util/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ export * from './autoBindDebug'
export * from './cached'
export * from './config-file-updater'
export * from './file'
export * from './hasTypescript'
export * from './pluginHandlers'
export * from './urqlCacheKeys'
18 changes: 18 additions & 0 deletions packages/data-context/test/unit/util/hasTypescript.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { expect } from 'chai'
import path from 'path'
import { hasTypeScriptInstalled } from '../../../src/util'
import { scaffoldMigrationProject } from '../helper'

describe('hasTypeScript', () => {
it('returns true when installed', async () => {
const monorepoRoot = path.join(__dirname, '..', '..', '..', '..', '..')

expect(hasTypeScriptInstalled(monorepoRoot)).to.be.true
})

it('returns false when not installed', async () => {
const projectRoot = await scaffoldMigrationProject('config-with-js')

expect(hasTypeScriptInstalled(projectRoot)).to.be.false
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ describe('Launchpad: Error System Tests', () => {
cy.contains('h1', cy.i18n.launchpadErrors.generic.configErrorTitle)
cy.percySnapshot()

cy.get('[data-testid="error-code-frame"]').should('contain', 'cypress.config.ts:6:9')
cy.get('[data-testid="error-code-frame"]').should('contain', 'cypress.config.ts:6:10')
})
})

Expand Down
2 changes: 1 addition & 1 deletion packages/launchpad/cypress/e2e/error-handling.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('Error handling', () => {
cy.contains('Choose a Browser').should('not.exist')

cy.withCtx((ctx) => {
ctx.actions.file.writeFileInProject('cypress.config.js', `
ctx.actions.file.writeFileInProject('cypress.config.ts', `
import { defineConfig } from 'cypress'
import { defineConfig as viteConfig } from 'vite'
export default defineConfig({
Expand Down
16 changes: 16 additions & 0 deletions packages/server/lib/plugins/child/register_ts_node.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const minimist = require('minimist')
const debugLib = require('debug')
const { register } = require('./ts_node')

const debug = debugLib('cypress:server:register-ts-node')

const args = minimist(process.argv)

debug('executing register_ts_node with args %o', args)

const { projectRoot, file } = args

if (projectRoot && file) {
debug('registering ts-node for projectRoot: %s and file: %s', projectRoot, file)
register(projectRoot, file)
}
45 changes: 11 additions & 34 deletions packages/server/lib/plugins/child/run_require_async_child.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
require('graceful-fs').gracefulify(require('fs'))
const stripAnsi = require('strip-ansi')
const debug = require('debug')(`cypress:lifecycle:child:run_require_async_child:${process.pid}`)
const debugLib = require('debug')
const { pathToFileURL } = require('url')
const tsNodeUtil = require('./ts_node')
const util = require('../util')
const { RunPlugins } = require('./run_plugins')

let tsRegistered = false
const debug = debugLib(`cypress:lifecycle:child:run_require_async_child:${process.pid}`)

/**
* Executes and returns the passed `file` (usually `configFile`) file in the ipc `loadConfig` event
Expand All @@ -22,14 +21,6 @@ function run (ipc, file, projectRoot) {
throw new Error('Unexpected: projectRoot should be a string')
}

if (!tsRegistered) {
debug('register typescript for required file')
tsNodeUtil.register(projectRoot, file)

// ensure typescript is only registered once
tsRegistered = true
}

process.on('uncaughtException', (err) => {
debug('uncaught exception:', util.serializeError(err))
ipc.send('childProcess:unhandledError', util.serializeError(err))
Expand Down Expand Up @@ -92,14 +83,9 @@ function run (ipc, file, projectRoot) {
// Config file loading of modules is tested within
// system-tests/projects/config-cjs-and-esm/*
const loadFile = async (file) => {
// 1. Try loading the configFile
// 2. Catch the "ERR_REQUIRE_ESM" error
// 3. Check if esbuild is installed
// 3a. Yes: Use bundleRequire
// 3b. No: Continue through to `await import(configFile)`
// 4. Use node's dynamic import to import the configFile

try {
debug('Loading file %s', file)

return require(file)
} catch (err) {
if (!err.stack.includes('[ERR_REQUIRE_ESM]') && !err.stack.includes('SyntaxError: Cannot use import statement outside a module')) {
Expand All @@ -110,25 +96,16 @@ function run (ipc, file, projectRoot) {
debug('User is loading an ESM config file')

try {
debug('Trying to use esbuild to run their config file.')
Copy link
Contributor Author

@lmiller1990 lmiller1990 Jun 7, 2022

Choose a reason for hiding this comment

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

I've removed this for now, we can definitely integration w/ the user's esbuild, but I think that should be done separately, since there's some unknowns.

I've called out this code deletion and referenced it in the esbuild issue so we don't forget about it: #22074

cc @tgriesser since I think you have some ideas around if/how we integrate esbuild. I think this is a lower priority in general, since not that many people are using esbuild (without TS) vs those using TS with Cypress (who can happily use the ts-node workflow).

// We prefer doing this because it supports TypeScript files
require.resolve('esbuild', { paths: [process.cwd()] })
// We cannot replace the initial `require` with `await import` because
// Certain modules cannot be dynamically imported.
// pathToFileURL for windows interop: https://github.com/nodejs/node/issues/31710
const fileURL = pathToFileURL(file).href

debug(`They have esbuild, so we'll load the configFile via bundleRequire`)
const { bundleRequire } = require('bundle-require')
debug(`importing esm file %s`, fileURL)

return (await bundleRequire({ filepath: file })).mod
return await import(fileURL)
} catch (err) {
if (err.stack.includes(`Cannot find module 'esbuild'`)) {
debug(`User doesn't have esbuild. Going to use native node imports.`)

// We cannot replace the initial `require` with `await import` because
// Certain modules cannot be dynamically imported.

// pathToFileURL for windows interop: https://github.com/nodejs/node/issues/31710
return await import(pathToFileURL(file).href)
}

debug('error loading file via native Node.js module loader %s', err.message)
throw err
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
require('../../../spec_helper')

const tsNodeUtil = require('../../../../lib/plugins/child/ts_node')
const runRequireAsyncChild = require('../../../../lib/plugins/child/run_require_async_child')
const resolve = require('../../../../lib/util/resolve')

describe('lib/plugins/child/run_require_async_child', () => {
beforeEach(function () {
Expand All @@ -17,28 +15,6 @@ describe('lib/plugins/child/run_require_async_child', () => {
mockery.deregisterMock('@cypress/webpack-batteries-included-preprocessor')
})

describe('typescript registration', () => {
beforeEach(() => {
sinon.stub(tsNodeUtil, 'register')
sinon.stub(resolve, 'typescript').returns('/path/to/typescript.js')
})

it('registers ts-node only once when typescript module found', function () {
runRequireAsyncChild(this.ipc, 'cypress.config.js', 'proj-root')
runRequireAsyncChild(this.ipc, 'cypress.config.js', 'proj-root')

expect(tsNodeUtil.register).to.be.calledWith(
'proj-root',
'cypress.config.js',
)

expect(tsNodeUtil.register).to.be.calledOnce
})

// FIXME: need to validate that TS is checked once when ts is not found as well
it.skip('checks for typescript only once if typescript module was not found')
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't register ts-node in the child process anymore, instead, we run the child process with --require server/lib...../register_ts_node. This also means we don't need the check to avoid the double registration anymore.

describe('errors', () => {
beforeEach(function () {
sinon.stub(process, 'on')
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"devDependencies": {
"typescript": "4.7.3"
},
"projectFixtureDirectory": "simple_passing"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


[email protected]:
version "4.7.3"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.7.3.tgz#8364b502d5257b540f9de4c40be84c98e23a129d"
integrity sha512-WOkT3XYvrpXx4vMMqlD+8R8R37fZkjyLGlxavMc4iB8lrl8L0DeTcHbYgw/v0N/z9wAFsgBhcsF0ruoySS22mA==
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"projectFixtureDirectory": "simple_passing"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{
"type": "module"
"type": "module",
"projectFixtureDirectory": "simple_passing"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"devDependencies": {
"typescript": "4.7.3"
},
"projectFixtureDirectory": "simple_passing"
}

This file was deleted.

Loading