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

Error with Winston library #58

Closed
maxime1992 opened this issue Mar 13, 2019 · 11 comments
Closed

Error with Winston library #58

maxime1992 opened this issue Mar 13, 2019 · 11 comments

Comments

@maxime1992
Copy link

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 🤷‍♂️. Any idea? I might be missing something obvious too...

Thanks!

@Robdel12
Copy link
Contributor

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

@maxime1992
Copy link
Author

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
Copy link
Contributor

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: webpack-contrib/css-loader#447 (comment)

@maxime1992
Copy link
Author

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

@Robdel12
Copy link
Contributor

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
Copy link

bahmutov commented Apr 1, 2019

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:

git clone git@github.com:bahmutov/calculator.git
cd calculator
git checkout add-percy
npx cypress open

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
Copy link
Contributor

Robdel12 commented Apr 2, 2019

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: cypress-io/cypress#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 added a commit to percy/percy-agent that referenced this issue Apr 2, 2019
This is to resolve the error where winston was being being included in
various webpack bundles. We change the `main` entry point of the
`@percy/agent` package so only the file that's ready for a browser is
bundled. Then we transpile that file fully with babel and rollup so
their webpack config isn't looking to compile it too.

This solves percy/percy-cypress#58 fully.
Robdel12 added a commit to percy/percy-agent that referenced this issue Apr 3, 2019
This is to resolve the error where winston was being being included in
various webpack bundles. We change the `main` entry point of the
`@percy/agent` package so only the file that's ready for a browser is
bundled. Then we transpile that file fully with babel and rollup so
their webpack config isn't looking to compile it too.

This solves percy/percy-cypress#58 fully.
Robdel12 added a commit to percy/percy-agent that referenced this issue Apr 3, 2019
This is to resolve the error where winston was being being included in
various webpack bundles. We change the `main` entry point of the
`@percy/agent` package so only the file that's ready for a browser is
bundled. Then we transpile that file fully with babel and rollup so
their webpack config isn't looking to compile it too.

This solves percy/percy-cypress#58 fully.
@Robdel12
Copy link
Contributor

Robdel12 commented Apr 3, 2019

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:

bahmutov/calculator@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
Copy link

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

@bahmutov
Copy link

bahmutov commented Apr 4, 2019

nice, works for my calculator demo, thank you

Robdel12 added a commit to percy/percy-agent that referenced this issue Apr 5, 2019
This is to resolve the error where winston was being being included in
various webpack bundles. We change the `main` entry point of the
`@percy/agent` package so only the file that's ready for a browser is
bundled. Then we transpile that file fully with babel and rollup so
their webpack config isn't looking to compile it too.

This solves percy/percy-cypress#58 fully.
Robdel12 added a commit to percy/percy-agent that referenced this issue Apr 5, 2019
* Introduce rollup config

This is to resolve the error where winston was being being included in
various webpack bundles. We change the `main` entry point of the
`@percy/agent` package so only the file that's ready for a browser is
bundled. Then we transpile that file fully with babel and rollup so
their webpack config isn't looking to compile it too.

This solves percy/percy-cypress#58 fully.

* Remove browser field, remove logger, test in other SDKs

Most of the errors stemmed from us using the logger (which is a node
logger) in our SDK utils, which is apart of our main export of the
package. This would cause bundlers to try and bundle that code and
fail because it couldn't find FS

* Remove changes to `sdk-utils.ts` file

That will be handled in a different PR and we're changing the way SDKs
import that file
djones pushed a commit that referenced this issue Apr 9, 2019
## [1.0.6](v1.0.5...v1.0.6) (2019-04-09)

### Bug Fixes

* Require @percy/agent 0.2.2. Resolves issue [#58](#58) ([#80](#80)) ([15c59be](15c59be))
@Robdel12
Copy link
Contributor

Robdel12 commented Apr 9, 2019

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

@Robdel12 Robdel12 closed this as completed Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants