Skip to content

.babelrc settings cached inappropriately #872

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

Closed
okcoker opened this issue May 25, 2016 · 16 comments
Closed

.babelrc settings cached inappropriately #872

okcoker opened this issue May 25, 2016 · 16 comments
Labels
bug current functionality does not work as desired

Comments

@okcoker
Copy link

okcoker commented May 25, 2016

I was going through the code coverage docs noticed that adding some of the params to my .babelrc file, I would get the error below when running tests:

# I've replaced some of the exact folder/file paths
> NODE_PATH=$NODE_PATH:./src nyc ava | tap-diff

test.js:1
(function (exports, require, module, __filename, __dirname) { import hi from 'methods/hi';
                                                              ^^^^^^
SyntaxError: Unexpected token import
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:511:25)
    at Module.replacementCompile (/project/node_modules/nyc/node_modules/append-transform/index.js:58:13)
    at extensions.(anonymous function) (/project/node_modules/require-precompiled/index.js:13:11)
    at /project/node_modules/nyc/node_modules/append-transform/index.js:62:4
    at require.extensions.(anonymous function) (/project/node_modules/ava/lib/test-worker.js:91:3)
    at Object.<anonymous> (/project/node_modules/nyc/node_modules/append-transform/index.js:62:4)
    at Module.load (module.js:456:32)
    at tryModuleLoad (module.js:415:12)
    at Function.Module._load (module.js:407:3)
    at Module.require (module.js:466:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/project/node_modules/ava/lib/test-worker.js:95:1)
    at Module._compile (module.js:541:32)
    at Module.replacementCompile (/project/node_modules/nyc/node_modules/append-transform/index.js:58:13)
    at module.exports (/project/node_modules/nyc/node_modules/default-require-extensions/js.js:8:9)

  test/test.js exited with a non-zero exit code: 1
npm ERR! Test failed.  See above for more details.

The thing is, changing my .babelrc file back to a simple config below and re-testing, it still failed:

{
    "presets": ["es2015", "stage-0"],
}

Clearing the node_modules folder fixed things it seemed. I thought maybe it was related to this so I started looking for cache problems. I noticed when I clear out the node_modules/.cache/ava/ folder and re-test with my simple .babelrc file, the error goes away and tests run fine.

AVA portion of package.json

"ava": {
    "files": [
      "test/**/*-test.js"
    ],
    "source": [
      "src/**/*.js"
    ],
    "failFast": true,
    "tap": true,
    "require": [
      "babel-register"
    ],
    "babel": "inherit"
  }

Dependencies:

  "devDependencies": {
    "ava": "^0.15.0",
    "babel-cli": "^6.9.0",
    "babel-eslint": "^6.0.4",
    "babel-preset-es2015": "^6.9.0",
    "babel-preset-stage-0": "^6.5.0",
    "babel-register": "^6.9.0",
    "eslint": "^2.10.2",
    "eslint-plugin-promise": "^1.1.0",
    "mocha": "^2.5.2",
    "nodemon": "^1.9.2",
    "nyc": "^6.4.4",
    "rimraf": "^2.5.2",
    "sinon": "^1.17.4",
    "tap-diff": "^0.1.1"
  }

Version info:

$ npm -v
3.8.6
$ node -v
v6.1.0
$ ava --version
0.15.0
@jamestalmage
Copy link
Contributor

Perhaps we should include the "babel" section of your package.json in the hash

@okcoker
Copy link
Author

okcoker commented May 25, 2016

I have no babel section. Unless of course you mean the dependencies in which case I've updated the original post

@novemberborn
Copy link
Member

Yea for this to work properly we'd have to resolve the Babel config and use that as part of the salt for the cache.

Alternatively we could print warnings when we detect Babel config inheritance/extension, with instructions on how to clear the cache.

@okcoker
Copy link
Author

okcoker commented May 25, 2016

@novemberborn I think even a warning/instructions would be a good first step. I was removing my node_modules and changing a bunch of things back and forth for a while, not knowing what I actually had done wrong or right.

@jamestalmage
Copy link
Contributor

I think this is a good argument for moving cache top level (.cache/ava), by forcing people to explicitly ignore it, you are making them aware of it.

@novemberborn
Copy link
Member

@jamestalmage makes sense. Or a CLI command to wipe it?

Note that the cache salt already includes the entire package.json, so it's really the .babelrc condition and further extends that we can't easily solve. Perhaps Babel exposes its configuration loader but IMO that's out of our scope.

@sindresorhus
Copy link
Member

@jamestalmage I don't think that will help much. You ignore the cache directory when adding AVA to a project and then forget. Later you experience a cache bug or another team member experiences it, and neither realizes it's a cache issue. Instead, I think we should do a good job as possible to hash correctly, even Babel config chains, and when it's absolutely not possible, print a warning. We could maybe also improve the documentation about clearing the cache on issues (even in the issue_template.md) and add a --clear-cache flag.

@vadimdemedes vadimdemedes added the bug current functionality does not work as desired label May 28, 2016
@danielkcz
Copy link

danielkcz commented Jun 20, 2016

I must say this was great source of frustration for me as well. At certain point I was sure that my config is correct, but tests runs were still saying it's wrong all over again. I ended up disabling cache while I was configuring when I found out about it.

Perhaps disabling cache by default might be another way to this and add there a warning that cache is disabled and how to enable it to improve performance. That way you are making user aware that cache is there. Sounds better to me then trying to cover whole babel configuration chain.

@novemberborn
Copy link
Member

Perhaps disabling cache by default might be another way to this and add there a warning that cache is disabled and how to enable it to improve performance. That way you are making user aware that cache is there.

AVA should be fast out of the box and not require more configuration in order to improve performance. And as @sindresorhus said regarding moving the cache directory next to the package.json:

I don't think that will help much. You ignore the cache directory when adding AVA to a project and then forget. Later you experience a cache bug or another team member experiences it, and neither realizes it's a cache issue.


So let's focus on:

  • Resolving the actual Babel config as it's applied to the test files and using that as part of the cache hash
  • Improving documentation
  • Adding a command to clear the cache

@jamestalmage
Copy link
Contributor

Resolving the actual Babel config as it's applied to the test files and using that as part of the cache hash

It won't be easy https://phabricator.babeljs.io/T6709

@jescalan
Copy link

jescalan commented May 1, 2017

So, for those of us stuck with this issue in the meantime, how would one clear the cache manually?

@sindresorhus
Copy link
Member

@jescalan rm -rf node_modules/.cache

@novemberborn
Copy link
Member

@jescalan what AVA version are you using? We should be able to pick up Babel config changes much better with 0.19.0.

@jescalan
Copy link

jescalan commented May 1, 2017

@novemberborn on the latest version, 0.19.1

@novemberborn
Copy link
Member

@jescalan could you share what changes you're making that are not causing test files to be recompiled?

@novemberborn
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired
Projects
None yet
Development

No branches or pull requests

7 participants