Skip to content

Clear the require cache before overriding fs functions - #141

Merged
tschaub merged 3 commits into
masterfrom
greedy
Jul 9, 2016
Merged

Clear the require cache before overriding fs functions#141
tschaub merged 3 commits into
masterfrom
greedy

Conversation

@tschaub

@tschaub tschaub commented Jul 9, 2016

Copy link
Copy Markdown
Owner

This makes things work in cases where another module requires fs-extra (or similar) before mocks-fs is required.

Fixes #103.

@gyandeeps

Copy link
Copy Markdown

Is this a breaking change?

@tschaub

tschaub commented Oct 28, 2016

Copy link
Copy Markdown
Owner Author

@gyandeeps - it should not be. Is there an easy way to reproduce the failure you were seeing?

@gyandeeps

Copy link
Copy Markdown

Basically eslint unit test started failing with 3.12.0 release. but if I comment out the lines changes in this commit (the cache changes) then it works fine. In eslint we use mock-fs only in 2 modules but all others use real files as of now. But in test which uses mock-fs we do make sure it calls the restore function also.

To be honest I havent bisected a lot to findout the real cause.

@tschaub

tschaub commented Oct 28, 2016

Copy link
Copy Markdown
Owner Author

Apologies for the trouble @gyandeeps. I'll try to find time to dig into the failures and make sense of what's going on. If any other reports come up, or if you would benefit from running tests on Node 7, I'll cut a release that reverts this change.

@gyandeeps

Copy link
Copy Markdown

Thanks for help @tschaub . We are planning to start doing node7 testing and for that we need to update the mock-fs. We would like a release of mock-fs with node7 updates but I would totally understand if you have reservations on it. If we can solve this in a easy way i am all of it.
Thanks again for support.

@not-an-aardvark

not-an-aardvark commented Oct 30, 2016

Copy link
Copy Markdown
Contributor

Hi, thanks for looking into this. I've also been investigating the eslint build failures. Here's a reproduction case that is a simplification of what our build is doing:

// test.js

require('./foo.js').modifyValue();

require('mock-fs');

assert(require('./foo.js').getValue() === true);
// foo.js

var value = false;

module.exports = {
  modifyValue() {
    value = true;
  },
  getValue() {
    return value;
  }
};

This works with mock-fs@3.11.0 and not with mock-fs@3.12.0, because mock-fs@3.12.0 clears the require cache when it loads.

@not-an-aardvark

Copy link
Copy Markdown
Contributor

I think #103 was occurring due to something like this:

// in fs-extra

module.exports = {
  readFile: require('fs').readFile
};

Since fs-extra has a reference to the readFile function rather than just the fs object, mutating the fs object wasn't changing the behavior of the fs-extra function.

As an alternative to clearing the require cache, have you considered mutating the process.binding('fs') object directly? All the fs functions depend on the process.binding('fs') object, so this would allow you to modify the behavior of the fs.readFile function itself, even if another module already had a reference to the function.

not-an-aardvark added a commit to eslint/eslint that referenced this pull request Oct 30, 2016
Due to a change in mock-fs 3.12.0, the `require` cache is cleared as soon as the mock-fs library is loaded (see tschaub/mock-fs#141). Our `lib/cli-engine.js` test uses `const Plugins = require('../../lib/plugins.js')` at the top level, and then some of the later tests use `require('mock-fs')` at the top level. Some of the `lib/cli-engine.js` tests have a requirement that `Plugins === require('../../lib/plugins.js')`, so using `require('mock-fs')` was causing the `require('../../lib/plugins.js')` value to change, which caused the tests to fail.

This commit fixes the issue by moving the `const Plugins = require('../../lib/plugins.js')` into a `before` hook, so that it gets run *after* `mock-fs` is loaded.
@tschaub

tschaub commented Nov 1, 2016

Copy link
Copy Markdown
Owner Author

@gyandeeps and @not-an-aardvark - I've reverted these changes and published mock-fs@3.12.1.

@not-an-aardvark - yes, modifying process.binding('fs') directly would be a nice alternative. It would be good to experiment with that approach.

@tschaub

tschaub commented Nov 1, 2016

Copy link
Copy Markdown
Owner Author

@not-an-aardvark - See #182 for a work-in-progress PR that overrides only process.binding('fs'). This is a nice simplification, but comes with some breaking changes. It might be possible to avoid these with a bit more work though.

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 this pull request may close these issues.

3 participants