percy / percy-cypress

Visual testing with Cypress and Percy
https://percy.io
MIT License
346 stars 40 forks source link

Error with Winston library #58

Closed maxime1992 closed 5 years ago

maxime1992 commented 5 years ago

I'm currently trying to setup Percy into Cypress, following this tutorial: https://docs.percy.io/docs/cypress

I then get an error Can't resolve fs.... Here's the stacktrace:

./node_modules/winston/lib/winston/common.js Module not found: Error: Can't resolve 'fs' in '/home/maxime/Documents/code/frontend/node_modules/winston/lib/winston' resolve 'fs' in '/home/maxime/Documents/code/frontend/node_modules/winston/lib/winston' Parsed request is a module using description file: /home/maxime/Documents/code/frontend/node_modules/winston/package.json (relative path: ./lib/winston) Field 'browser' doesn't contain a valid alias configuration resolve as module /home/maxime/Documents/code/frontend/node_modules/winston/lib/winston/node_modules doesn't exist or is not a directory /home/maxime/Documents/code/frontend/node_modules/winston/lib/node_modules doesn't exist or is not a directory /home/maxime/Documents/code/frontend/node_modules/node_modules doesn't exist or is not a directory /home/maxime/Documents/code/node_modules doesn't exist or is not a directory /home/maxime/Documents/node_modules doesn't exist or is not a directory /home/maxime/node_modules doesn't exist or is not a directory /home/node_modules doesn't exist or is not a directory /node_modules doesn't exist or is not a directory looking for modules in /home/maxime/Documents/code/frontend/node_modules/winston/node_modules using description file: /home/maxime/Documents/code/frontend/node_modules/winston/package.json (relative path: ./node_modules) Field 'browser' doesn't contain a valid alias configuration looking for modules in /home/maxime/Documents/code/frontend/node_modules using description file: /home/maxime/Documents/code/frontend/package.json (relative path: ./node_modules) Field 'browser' doesn't contain a valid alias configuration using description file: /home/maxime/Documents/code/frontend/node_modules/winston/package.json (relative path: ./node_modules/fs) no extension Field 'browser' doesn't contain a valid alias configuration using description file: /home/maxime/Documents/code/frontend/package.json (relative path: ./node_modules/fs) no extension Field 'browser' doesn't contain a valid alias configuration /home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs doesn't exist .ts Field 'browser' doesn't contain a valid alias configuration /home/maxime/Documents/code/frontend/node_modules/fs doesn't exist .ts Field 'browser' doesn't contain a valid alias configuration /home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs.ts doesn't exist .js Field 'browser' doesn't contain a valid alias configuration /home/maxime/Documents/code/frontend/node_modules/fs.ts doesn't exist .js Field 'browser' doesn't contain a valid alias configuration /home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs.js doesn't exist /home/maxime/Documents/code/frontend/node_modules/fs.js doesn't exist as directory /home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs doesn't exist as directory /home/maxime/Documents/code/frontend/node_modules/fs doesn't exist [/home/maxime/Documents/code/frontend/node_modules/winston/lib/winston/node_modules] [/home/maxime/Documents/code/frontend/node_modules/winston/lib/node_modules] [/home/maxime/Documents/code/frontend/node_modules/node_modules] [/home/maxime/Documents/code/node_modules] [/home/maxime/Documents/node_modules] [/home/maxime/node_modules] [/home/node_modules] [/node_modules] [/home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs] [/home/maxime/Documents/code/frontend/node_modules/fs] [/home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs.ts] [/home/maxime/Documents/code/frontend/node_modules/fs.ts] [/home/maxime/Documents/code/frontend/node_modules/winston/node_modules/fs.js] [/home/maxime/Documents/code/frontend/node_modules/fs.js] @ ./node_modules/winston/lib/winston/common.js 12:9-22 @ ./node_modules/winston/lib/winston.js @ ./node_modules/@percy/agent/dist/utils/logger.js @ ./node_modules/@percy/agent/dist/utils/sdk-utils.js @ ./node_modules/@percy/agent/dist/index.js @ ./node_modules/@percy/cypress/dist/index.js @ ./cypress/support/commands.js @ ./cypress/support/index.js

I've done import '@percy/cypress'; in command.js file and that's pretty much it :man_shrugging:. Any idea? I might be missing something obvious too...

Thanks!

Robdel12 commented 5 years ago

I've seen this come up -- are you using custom preprocessing with Cypress? Like with webpack?

maxime1992 commented 5 years ago

Well, we used the Typescript plugin made by Gleb and so we, indeed, have got a webpack conf like the following:

const TsconfigPathsPlugin = require('tsconfig-paths-webpack-plugin');

module.exports = {
  resolve: {
    extensions: ['.ts', '.js'],
    plugins: [
      new TsconfigPathsPlugin({
        configFile: 'src/tsconfig.e2e.json',
      }),
    ],
  },
  module: {
    rules: [
      {
        test: /\.ts$/,
        exclude: [/node_modules/],
        use: [
          {
            loader: 'ts-loader',
            options: {
              configFile: 'tsconfig.e2e.json',
            },
          },
        ],
      },
    ],
  },
};

But I don't think we've made any customization to that file at all.

Robdel12 commented 5 years ago

Gotcha, this is helpful. The error is related to Winston being transpiled when it's a node library. We don't see those issues on our example app since we're not doing anything like that.

So! I'm going to take that and figure out what needs to be done to fix that. Off the top of my head, I'd be curious if this would be a suitable workaround: https://github.com/webpack-contrib/css-loader/issues/447#issuecomment-285598881

maxime1992 commented 5 years ago

@Robdel12 just had the chance to try and it's indeed solved by adding

module.exports = {
+ node: {
+   fs: 'empty'
+ },
  resolve: {

Thanks for your help! There should be a better fix but in the meantime I'll be able to use Percy :)

If you plan to fix it, keep the issue opened otherwise feel free to close it :+1:

Robdel12 commented 5 years ago

Sweet! That's good to know, I'm going to keep this open and see if we can find a better solution for this.

bahmutov commented 5 years ago

Does this solve this though? I am trying to test cypress-react-unit-test using the plugin and while node: {fs: 'empty'} allows winston to load, the check below now is invalid and tries to do cy.exec 103 5338 :)

/ An attempt to resiliently find the path to the 'percy-healthcheck' script, and to do so
// in a cross-platform manner.
function _healthcheckPath(options) {
    try {
        // Try to resolve with respect to the install local module.
        return require.resolve('@percy/cypress/dist/percy-healthcheck');
    }
    catch (_a) {
        try {
            // Try to resolve relative to the current file.
            return require.resolve('./percy-healthcheck');
        }
        catch (_b) {
            // Oh well. Assume it's in the local node_modules.
            // It would be nice to use __dirname here, but this code is entirely executed inside of
            // Cypress' unusual context, making __dirname always '/dist' for this file, which is
            // unhelpful when trying to find a working filesystem path to percy-healthcheck.
            return path.join('.', './node_modules/@percy/cypress/dist/percy-healthcheck');
        }
    }
}

with fs: empty the require.resolve('@percy/cypress/dist/percy-healthcheck') is transpiled by the webpack like (103) like literally :(

Screen Shot 2019-04-01 at 6 14 07 PM

I got around this in https://percy.io/bahmutov/calculator by running locally and adding an option to

function _healthcheckPath(options) {
    if (options.percyFromNodeModules) {
        return path.join('.', './node_modules/@percy/cypress/dist/percy-healthcheck');
    }
    ...
}

and it would be nice to expose this as a user option

var healthcheck = "node " + _healthcheckPath(options) + " " + percyAgentClient.port;

To recreate the problem:

and you will see the problem

Here is how the healthcheck is transpiled by the way when webpack option is set to node: { fs: 'empty' }

Screen Shot 2019-04-01 at 9 54 57 PM
Robdel12 commented 5 years ago

Yeah, I had a feeling introducing a ~FS~ path into the library would complicate things more. We have to do this whole dance until we can figure out a solution here: https://github.com/cypress-io/cypress/issues/3161

I'm going to take a look at your PR + open a PR on percy-agent that removes winston from being transpiled for the browser

Robdel12 commented 5 years ago

Hey @bahmutov & everyone here,

We've been hard at work to solve that issue over the past few days. I'm working on a change that will get rid of adding this from your configs:

node: {
  fs: 'empty'
},

What is happening is webpack is trying to bundle our node code with the small amount of browser code that we have in @percy/agent. The way to fix this is to properly bundle & transpile the code we inject into the browser & introduce a browser field in the package.json of @percy/agent. That way the bundlers will see the browser field and only bundle that code together. Right now our main in the package.json points to the entry of our node app, which bundlers use and try to bundle together 💥 . Once that's done you'll be able to remove that workaround from your webpack configs.

For @bahmutov (& @mapsandapps) regarding the health check issue with webpack, we shipped a new version of this SDK that should clear that right up (@percy/cypress v1.0.5). I took the calculator example and updated it:

https://github.com/bahmutov/calculator/compare/add-percy...Robdel12:rd/add-percy

I'll also be using that as a test for the bundling fixes I talked about earlier. I'd love to hear if the new version of the SDK unblocks y'all!

mapsandapps commented 5 years ago

v1.0.5 seems to be working. Thanks for the fix!

bahmutov commented 5 years ago

nice, works for my calculator demo, thank you

Robdel12 commented 5 years ago

For those that had the workaround their webpack configs to exclude fs:

module.exports = {
+ node: {
+   fs: 'empty'
+ },
  resolve: {

You can remove that now with the latest version of the @percy/cypress package (v1.0.6). https://github.com/percy/percy-cypress/blob/master/CHANGELOG.md#106-2019-04-09