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

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Jun 6, 2022

User facing changelog

Support TypeScript projects using "type": "module" using ts-node/esm.

Additional details

The problem is if you are using "type": "module", Node.js will use it's ESM loader to load your code. If you are using a ts extension, which Node.js does not know about out of the box, you get the various errors we've seen reported.

The original solution was to use esbuild to try load TS files if the initial require('cypress.config...) failed, but it's not working as expected, as explained here. I removed the esbuild code path for now. I noted this here so we can bring it back in the future, if needed.

The solution introduced here is to use the ESM support that ts-node ships, as recommended in #21939. We rely on ts-node in the project and already ship it, so it makes sense to continue to use it where we can.

Steps to test

  1. Reproduce Initializing Cypress v10 on a "type": "module" project with TypeScript results in an [ERR_UNKNOWN_FILE_EXTENSION] error #22096 with the latest binary from npm and verify problem
  2. Grab this branch and build the binary locally
  3. Open the repro with the new binary, it should work now

How has the user experience changed?

Can correctly load cypress.config.ts for "type": "module" projects.

The workflow is now:

image

The old workflow (shows problem):

Show old workflow diagram image

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?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 6, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jun 6, 2022



Test summary

37553 0 454 0Flakiness 3


Run details

Project cypress
Status Passed
Commit 27bf091
Started Jun 9, 2022 9:54 AM
Ended Jun 9, 2022 10:10 AM
Duration 16:34 💡
OS Linux Debian - 10.11
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/actions/click.cy.js Flakiness
1 ... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
cypress/proxy-logging.cy.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
e2e/origin/commands/assertions.cy.ts Flakiness
1 cy.origin assertions > #consoleProps > .should() and .and()

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

@lmiller1990 lmiller1990 marked this pull request as ready for review June 6, 2022 11:17
@lmiller1990 lmiller1990 requested review from a team and tgriesser as code owners June 6, 2022 11:17
@lmiller1990 lmiller1990 requested review from jennifer-shehane, a team, tgriesser and JessicaSachs and removed request for a team, tgriesser and jennifer-shehane June 6, 2022 11:17
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.

@@ -1,4 +1,3 @@
// @ts-check
// this module is responsible for loading the plugins file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason this caused errors to be raised when using ts-node/esm, I don't quite know why, so I just removed the @ts-check. These files are not very type safe anyway, I don't think the ts-check is doing too much with the very lax tsconfig.json in packages/server, anyway. We can work on making it nice and strict in the future.

Copy link
Member

Choose a reason for hiding this comment

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

@mjhenkes mjhenkes linked an issue Jun 6, 2022 that may be closed by this pull request
@@ -238,6 +240,37 @@ 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

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.

@baus baus added the PATCH label Jun 6, 2022
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

I'm a bit confused with how we are dealing with esm. This PR adds the ts-node/esmloader to where we start the config process, but we still register ts-node in the child process and then still use esbuild to require the user's config. We now have three places where configure ts/esm...
Can we not have one place for all this ts/esm logic?

@jennifer-shehane jennifer-shehane removed their request for review June 6, 2022 21:25
@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 7, 2022

@ZachJW34 good point on the "register ts-node/esm then register ts-node" double registration.

I think we should just run the entire sub-process with ts-node or ts-node/esm - one place to do the ts-node things. I'll see if there are any downsides to this. I made that change, take a look. I also added a diagram to help.

As for esbuild, as alluded to #22074 and #22118 (comment), that code path actually doesn't do anything in prod (a bug which we did not realize until now).

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 7, 2022

@ZachJW34 I tried to improve the module loading logic and move it to one place. Here's what I came up with:

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
}
}
const proc = fork(CHILD_PROCESS_FILE_PATH, configProcessArgs, childOptions)

Basically:

if (has typescript) {
  if (esModules) {
    NODE_OPTIONS = `--loader ts-node/esm`
  } else {
    // we have some custom logic for our `ts_node` util, so we need to --require
    // it so the code executes (cannot do this with `--loader`).
    // I am not sure if we need it for `ts-node/esm`.
    NODE_OPTIONS = `--require @packages/server/..../register_ts_node` 
  }
} else {
  // typescript not available, cannot use ts-node.
  // just use native Node.js loader
}

Now instead of registering ts-node by calling tsNodeUtil.register() in the run_require_async_child, we register it using Node's --require hook (basically the same thing, from my understanding). Using --require is one of the two ways discussed in their docs, along with --loader (more experimental, but that's how they recommend using their ts-node/esm code).

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

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

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

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 9, 2022

I spent most of the day dealing with windows bugs. I ended up reverting a few things, seems like it's mostly fine now (albeit a little CI flake).

@lmiller1990 lmiller1990 merged commit abd986a into develop Jun 9, 2022
@lmiller1990 lmiller1990 deleted the lmiller1990/21939 branch June 9, 2022 11:20
flotwig added a commit that referenced this pull request Jun 9, 2022
@lmiller1990 lmiller1990 restored the lmiller1990/21939 branch June 10, 2022 05:03
@lmiller1990 lmiller1990 mentioned this pull request Jun 10, 2022
1 task
ryanthemanuel added a commit that referenced this pull request Jun 10, 2022
@mjhenkes mjhenkes removed the PATCH label Jun 10, 2022
@chinchang
Copy link

chinchang commented Jun 13, 2022

Even after this fix, I still get Your configFile is invalid: /home/runner/work/..../cypress.config.ts error.
Are there any post-upgrade changes required to support a ts project?

I am using this on a nextJS project. It doesn't/can't have type:module in package.json.

@hanna-becker
Copy link

hanna-becker commented Jun 13, 2022

We changed the target in cypress's tsconfig from "es5" to "es2019" to get it working.

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 14, 2022

Even after this fix, I still get Your configFile is invalid: /home/runner/work/..../cypress.config.ts error. Are there any post-upgrade changes required to support a ts project?

I am using this on a nextJS project. It doesn't/can't have type:module in package.json.

@chinchang can you open a new issue with a reproduction? This issue was specifically for ESM support, which means projects with "type": "module".

No changes should be required. If it's not working as expected, it's a bug and I will fix it for you. I've used Cypress 10 with Next.js projects before, but Next.js projects come in many different configurations - please share your reproduction in a new issue and tag me. 🙏 We are pushing out fixes as soon as they are ready 👍

Also @geoffrey-adeo - I see you 👍 the issue, if you are encountering a similar problem, please create an issue explaining how to reproduce and I can look into it.

BeijiYang pushed a commit to BeijiYang/cypress that referenced this pull request Jun 23, 2022
…io#22118)

* fix: support ESM projects using TypeScript with ts-node/esm

* better error handling

* fix test

* indentation

* register ts-node via --require hook

* be less aggressive with erroring

* update fix system tests

* remove obsolete test

* handle case of not using typescript

* replicate 9.x behavior for legacy plugins w/ ts-node

* make test project valid and adjust tests accordingly

* use ts-node/esm transpile only

* dummy

* extract util function

* merge in refactor using projectFixtureDirectory

* fix test
@a-churchill
Copy link

I also see the Your configFile is invalid error, with the message:

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" 
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:65:15)
    at Loader.getFormat (internal/modules/esm/loader.js:113:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:244:31)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Loader.import (internal/modules/esm/loader.js:178:17)

This is in a project that worked fine with v10.0.3; I made no changes other than upgrading to 10.3.0 and I see this issue.

We use yarn v2 and TypeScript - maybe this is related to yarn v2's different behavior with require.resolve/require?

Happy to give more info if it's helpful, let me know what I can do!

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jul 10, 2022

@a-churchill a minimal reproduction would be super useful. If you could provide that, I can definitely take a look. I'm surprised require.resolve is different in yarn 2 - this is a pretty core behavior in Node, what does yarn 2 do differently?

@andrei-dascalu
Copy link

In my case, with a default Cypress setup in an evironment that doesn't provide typescript will result in this error. If typescript is provided (either in locap package.json or linked globally), it's all good.
It's strange though that the official docker image of Cypress doesn't provide typescript globally since the default config is now TS.

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Nov 10, 2022

The default config is only TS if you have it installed, otherwise we default to JS.

A default setup should definitely not error. How do I repro - just create a new project, install Cypress, and open it? This works for me -- what Node.js version do you have?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet