standard-things / esm

Tomorrow's ECMAScript modules today!
Other
5.27k stars 147 forks source link

Something is wrong with a cache #805

Open theKashey opened 5 years ago

theKashey commented 5 years ago

Derived from #798, #799 and https://github.com/theKashey/rewiremock/pull/83

There is a simple test:

// B.js import C from './C'; export default C;

// C.js export default 42;


### Test 1
```js
 const subject = rewiremock.proxy(
      () => require('./A'),
      () => {
        rewiremock(() => require('./B')).with('bar')
      }
    );
    expect(subject.default).to.be.equal('bar')

In this test we are replacing B by "bar". It is working

Test 2

it('mock B', () => {
    const subject = rewiremock.proxy(
      () => require('./B'),
      () => {rewiremock(() => require('./C')).withDefault('baz')}
    );
    expect(subject.default).to.be.equal('baz')
  });

This time test would fail. C would be not mocked, as long as B cache would be reused, so it never gets a change to get mocked - execution enters ESM loader with "B", and exists with a result, never calls Module._load again.

Test 3

Add one more test, between these two

it('mock C', () => {
    const subject = rewiremock.proxy(
      () => require('./A'),
      () => {rewiremock(() => require('./C')).with('bar')}
    );
    expect(subject.default).to.be.equal('bar')
  });

It would fail, but test 2 which would be executed right after it, will pass.

Test 4

So - there is something like cache poisoning, which take a place after A.js

Add D.js

I've extended chain by D.js, then started modifying the code.

I've also tried to run rewiremocks tests vs esm and majority failed due to cache related issues.

I've also tried to disable ESM cache, but it has no effect.

Theory

Some parts of rewiremock uses require.cache, while others uses Module._cache. While it's the same for nodejs and webpack, it might not be the case for esm.

boneskull commented 5 years ago

I think the cache-disabling flag just prevents it from being loaded from disk. Stuff still goes in an in-memory cache

theKashey commented 5 years ago

Another tricky moment:

operation file parent parent of parent
require C.js A.js mock.spec.js
require B.js C.js A.js

Then I am adding esm

operation file parent parent of parent
require C.js A.js mock.spec.js
require B.js C.js mock.spec.js

From call to call the parent of C has changed. In nodejs a module could have only one parent, and 'B' was already "used" from "mock.spec", but that usage should not be cached, and any cache could mean nothing for Module._load level, as long as it exists before the cache line.

There is only one place which might set mock.spec as a parent of C.js - the test itself. To be more concrete - the guided mock construction - rewiremock(() => require('./C')).with('bar').

How it works:

Probably it's now easy to create a first test for a problem - overloading Module._load with nothing should disable all side effects on the cache system.

The best solution - move cache border one level down. It's quite hard to debug esm, but Module._load is called from esm and calling a "real" Module._load would dive to esm again. Cache border right now is at first point, while should be at the second.

boneskull commented 5 years ago

The TL;DR of this issue is that “esm and rewiremock do not play nice together” to such a degree that the combination isn’t feasible beyond trivial use of rewiremock.

boneskull commented 5 years ago

I think jdalton is on an open-source vacation / starting new job, so don’t expect the usual prompt fix.

theKashey commented 5 years ago

Look like scratchCache is the source of the problem. While I've found a way to debug esm(provide a correct filename for Script in esm.js) - I could not understand how it is populated - there is no logic between require and rewiremock.

jdalton commented 5 years ago

Hi @theKashey!

The scatchCache is used for the in-between phases of parsing and execution of ES modules. If you could create a small repro repo I can dig in and investigate a fix.

theKashey commented 5 years ago

This test https://github.com/theKashey/rewiremock/blob/master/_tests/mock.spec.js#L262-L274

👎 fail - node node_modules/mocha/bin/_mocha --require esm --ui bdd rewiremock/_tests/mock.spec.js --grep "^rewiremock called from a mock should mock only direct child due to callThrough mocked \/ async \: $"

👍 pass - node node_modules/mocha/bin/_mocha --compilers js:babel-core/register --ui bdd rewiremock/_tests/mock.spec.js --grep "^rewiremock called from a mock should mock only direct child due to callThrough mocked \/ async \: $"

I'll try to extract it in a shorter form, you will be able to integrate to esm's tests.

jdalton commented 5 years ago

@theKashey

I'll try to extract it in a shorter form, you will be able to integrate to esm's tests.

Thank you!

theKashey commented 5 years ago

https://github.com/theKashey/esm-bug-805

I've tried to make the example as complicated, as possible - but it just works, except this moment.

I've also update esm from 3.2.22 to 3.2.25 and look like THIS issue is resolved. At least the test I was playing with is green.