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

Never uses esbuild to process cypress.config #22074

Closed
lmiller1990 opened this issue Jun 3, 2022 · 5 comments · Fixed by #22118
Closed

Never uses esbuild to process cypress.config #22074

lmiller1990 opened this issue Jun 3, 2022 · 5 comments · Fixed by #22118
Assignees
Labels
pkg/server This is due to an issue in the packages/server directory type: bug v10.0.0 🐛 Issue present since 10.0.0

Comments

@lmiller1990
Copy link
Contributor

lmiller1990 commented Jun 3, 2022

Current behavior

Attempts to use esbuild but never actually does so successfully.

Details

EDIT: I removed the esbuild code path for now, as part of this PR #22118 where we use ts-node/esm to handle ES modules. You can reference that if/when we decided to work on this issue and revisit how we use esbuild in Cypress.

On this part of the code:

return (await bundleRequire({ filepath: file })).mod
} catch (err) {
if (err.stack.includes(`Cannot find module 'esbuild'`)) {

We do:

try {
  require.resolve('esbuild', { paths: [process.cwd()] })

  const { bundleRequire } = require('bundle-require')

  return (await bundleRequire({ filepath: file })).mod
} catch (err) {
  if (err.stack.includes(`Cannot find module 'esbuild'`)) {
    // ...
  }

  throw err
}

The problem is this will almost always fail - we check if esbuild is in the user's node_modules, via process.cwd (which is going to be projectRoot) but then require bundle-require from the binary's node_modules, which also assumes esbuild is present in the binary's node_modules (which it never is - we do not ship esbuild with the binary).

For this to work, we would need to esbuild with the binary. It's around 8MB:

/Users/lachlan/code/dump/cypress10-upgrade-issue
$ du -h node_modules/esbuild                                                                            
8.0M	node_modules/esbuild/bin
4.0K	node_modules/esbuild/node_modules/.bin
4.0K	node_modules/esbuild/node_modules
100K	node_modules/esbuild/lib
8.1M	node_modules/esbuild

Desired behavior

Either we ship esbuild or find some way to utilise the user's esbuild (cannot think of one). Or, just don't have this code (it's not doing anything).

It's also not clear why we even include esbuild - Node supports native ES modules now, I'm not sure this is really necessary.

Test code to reproduce

Here is the output.

  cypress:lifecycle:child:run_require_async_child:56066 User is loading an ESM config file +14ms
  cypress:lifecycle:child:run_require_async_child:56066 Trying to use esbuild to run their config file. +0ms
  cypress:lifecycle:child:run_require_async_child:56066 They have esbuild, so we'll load the configFile via bundleRequire +2ms
  cypress:lifecycle:child:run_require_async_child:56066 User doesn't have esbuild. Going to use native node imports. +9ms

Larger output:

$ /Users/lachlan/code/dump/cypress-mjs/node_modules/.bin/cypress open
  cypress:lifecycle:ProjectConfigIpc fork child process { CHILD_PROCESS_FILE_PATH: '/Users/lachlan/Library/Caches/Cypress/10.0.2/Cypress.app/Contents/Resources/app/packages/server/lib/plugins/child/require_async_child.js', configProcessArgs: [ '--projectRoot', '/Users/lachlan/code/dump/cypress-mjs', '--file', '/Users/lachlan/code/dump/cypress-mjs/cypress.config.js' ], childOptions: { stdio: 'pipe', cwd: '/Users/lachlan/code/dump/cypress-mjs', execPath: '/Users/lachlan/.nvm/versions/node/v16.14.2/bin/node' } } +0ms
  cypress:lifecycle:ProjectConfigIpc trigger the load of the file +8ms
  cypress:lifecycle:child:run_require_async_child:56066 configFile: /Users/lachlan/code/dump/cypress-mjs/cypress.config.js +0ms
  cypress:lifecycle:child:run_require_async_child:56066 projectRoot: /Users/lachlan/code/dump/cypress-mjs +1ms
  cypress:lifecycle:child:run_require_async_child:56066 register typescript for required file +0ms
  cypress:lifecycle:child:run_require_async_child:56066 try loading /Users/lachlan/code/dump/cypress-mjs/cypress.config.js +25ms
  cypress:lifecycle:child:run_require_async_child:56066 User is loading an ESM config file +14ms
  cypress:lifecycle:child:run_require_async_child:56066 Trying to use esbuild to run their config file. +0ms
  cypress:lifecycle:child:run_require_async_child:56066 They have esbuild, so we'll load the configFile via bundleRequire +2ms
  cypress:lifecycle:child:run_require_async_child:56066 User doesn't have esbuild. Going to use native node imports. +9ms
  cypress:lifecycle:child:run_require_async_child:56066 loaded config file /Users/lachlan/code/dump/cypress-mjs/cypress.config.js +257ms
  cypress:lifecycle:child:run_require_async_child:56066 loaded config from /Users/lachlan/code/dump/cypress-mjs/cypress.config.js { e2e: { setupNodeEvents: [Function: setupNodeEvents] } } +1ms
  cypress:lifecycle:ProjectConfigIpc loadConfig:reply +2s
  cypress:lifecycle:ProjectConfigManager config is loaded for file /Users/lachlan/code/dump/cypress-mjs/cypress.config.js null +0ms

Cypress Version

10.0.2

Other

No response

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 6, 2022

I did a lot of digging to explore solutions that will make Cypress as "batteries included" and "no config" as possible. Here's the current state of things, and my recommendations.

Problem

When we load the user's cypress.config, we do so in a child process. The child process is require_async_child.js, found here. We then register ts-node, that line is here... tsNodeUtil.register(projectRoot, file).

The process is originally forked here:

If users have "type": "module", the Node.js process will default to ESM. Of course, Node.js does not know about .ts, so when we eventually try to execute the cypress.config (code is here):

require('./cypress.config.ts`)
// Or, neither works right now
import './cypress.config.ts')

So, the problem is you cannot handle ts files using native Node.js ESM module API. Node.js does not understand the ts extension.

Ideal Solution: Use bundled ts-node with ESM support

Ideally, we would use the ESM support ts-node has, thread, since we already ship ts-node and some code to register it using the user's own TypeScript version.

This involves seeing if the user is using ESM, then executing the child process with NODE_OPTIONS--experimental-specifier-resolution=node --loader <loader>. The downside, obviously, is using an experimental flag, and the fact the ESM support is ts-node does have known bugs.

Edit: @tgriesser already identified this, I just saw this issue: #21939. Glad that we both arrived at the same solution.

Original Intended Solution

As noted in the original post for this issue, our original idea was to use esbuild if the user has it installed. esbuild can handle TS using ES Modules just fine.

The crux of the problem is this

try {
  debug('Trying to use esbuild to run their config file.')
  // We prefer doing this because it supports TypeScript files
  require.resolve('esbuild', { paths: [process.cwd()] })

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

  return (await bundleRequire({ filepath: file })).mod
} catch (err) {
  if (err.stack.includes(`Cannot find module 'esbuild'`)) {
    // use native ES module support 
}

We require.resolve('esbuild') - works fine, if they have it installed. The next line is the problem - const { bundleRequire } = require('bundle-require'). This is not require.resolve - we use the bundled bundle-require, which has esbuild as a dependency, which therefore also assumes esbuild is bundled in the binary, which it is not.

One alternative would be to patch the _load function to make sure it uses the user's esbuild binary, like we do in webpack (bit of a hack).

;(Module as ModuleClass)._load = function (request, parent, isMain) {
if (request === 'webpack' || request.startsWith('webpack/')) {
const resolvePath = require.resolve(request, {
paths: [webpack.importPath],
})
debug('Webpack: Module._load resolvePath - %s', resolvePath)
return originalModuleLoad(resolvePath, parent, isMain)
}
return originalModuleLoad(request, parent, isMain)
}

Recommendation

I think we should add support for ts-node/esm, like suggested in #21939.

Oustanding Questions

I'm not entirely clear on when we'd use esbuild over ts-node. Is there something esbuild does that ts-node/esm does not? Right now this code path is never going to get hit, so the "try to use esbuild" concept is dead code - it's also incredibly misleading, since the system tests actually do use esbuild, since they are not system binary tests. If they were running against the binary, they'd fail.

@mirobo
Copy link
Contributor

mirobo commented Jun 28, 2022

Oustanding Questions

I'm not entirely clear on when we'd use esbuild over ts-node. Is there something esbuild does that ts-node/esm does not? Right now this code path is never going to get hit, so the "try to use esbuild" concept is dead code - it's also incredibly misleading, since the system tests actually do use esbuild, since they are not system binary tests. If they were running against the binary, they'd fail.

We've used the integrated TypeScript transpiler at first, but it was becoming incredibly slow because of the amount of tests and dependencies we had. Once we switched to esbuild (using one of the two freely available npm plugins with minimal setup required), the preprocessing became extremely fast.

I'd be happy to see esbuild getting integrated in a working manner, as it's much more performant (at least for us). But then again, bundling esbuild might lock users to a specific version? The current solution with copy&pasted boilerplate code for the esbuild-preprocessor is not ideal, but it works. Maybe this preprocessor-code-snippet could be integrated so you'd only need to pass the esbuild-options..

@lmiller1990
Copy link
Contributor Author

I agree, esbuild is the bomb. Rather than waiting for us to ship something official I think we can build something similar to https://github.com/cypress-io/cypress/tree/develop/npm/webpack-preprocessor but as esbuild-preprocessor. Are you interested in working on something like this? We can def collaborate on a third party preprocessor that could likely grow into the official one.

I'd really like that, we can go way faster if we move away from ts-node to esbuild. I don't think this will be happening in core for a while, since many projects depend on how we use ts-node, but we can definitely build out something in the meantime as a plugin (which has access to all the internals you'd need anyway).

How are you currently using esbuild with Cypress?

@mirobo
Copy link
Contributor

mirobo commented Jun 29, 2022

I am interested, but honestly I don't have enough time to contribute seriously and the Cypress dev environment is pretty much impossible to install inside a corporate network (on my private PC it just worked). And after all I'm "just" a user...

First I tried this https://github.com/bahmutov/cypress-esbuild-preprocessor , but I can't remember what didn't work, so I just tried the other one ever since https://github.com/sod/cypress-esbuild-preprocessor with esbuild 0.14.29.

The setup is nothing more than already described:

const path = require('path');

module.exports = (on, config) => {
    on(
        'file:preprocessor',
        cypressEsbuildPreprocessor({
            esbuildOptions: {
                // optional tsconfig for typescript support and path mapping (see https://github.com/evanw/esbuild for all options)
                tsconfig: path.resolve(__dirname, '../../tsconfig.json'),
            },
        }),
    );
};

If I recall correctly we used the webpack-preprocessor at one time but had compatibility issues because our app used webpack 5.

I think it would be already a good practice to somehow maintain/fork this repo https://github.com/sod/cypress-esbuild-preprocessor and offer it as an official solution for preprocessing with esbuild.

If I understood correctly Cypress 10 should preprocess TypeScript out-of-the-box without any further configuration?

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Jun 29, 2022

TypeScript should work out of the box! You might need to configure tsconfig.json depending on what features you're using, but if you have TS in your code base already, it should "just work". You can see the docs for more info. If you turn off type checking (typescript has a transpileOnly feature iirc) that might speed things up. You could then run type checks in a separate process, or rely on your IDE.

I agree should contribute to https://docs.cypress.io/guides/tooling/typescript-support, or just fork - the source is only around 40 LOC. I'd like to explore this, but I can't commit to a specific date/timeframe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/server This is due to an issue in the packages/server directory type: bug v10.0.0 🐛 Issue present since 10.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants