Unable to mock `.mjs` files

theKashey commented 5 years ago

Let's define two files - foo and index. Then lets mock them using Module._load based mocker.

// foo.js
export default () => 'FOO';

// index.js
import foo from './foo';
export default () => foo();

Scenario 1 - all `.js`

mock foo.js to return bar
require foo.js - 👍
require index.js - 👍

Scenario 2 - `foo.mjs` + `index.js`

mock foo.mjs to return bar
require foo.mjs - 👍
require index.js - 👍

Scenario 3 - `foo.mjs` + `index.mjs`

mock foo.mjs to return bar
require foo.mjs - 👍
require index.mjs - 👎 <<---------, while foo.mjs is still mocked.

Scenario 3 - `foo.js` + `index.mjs`

Cannot load module (foo.js) from .mjs

Probably this is a feature, not a bug. But how then mock .mjs?

Example: https://github.com/theKashey/rewiremock-esm-repro

jdalton commented 5 years ago

Hi @theKashey!

Can you clarify things a bit more. It's not clear to me what is and isn't working based on your example repo and the scenario's you've posted. I don't know what "require foo.mjs" or "require index.js" means in this scenario.

theKashey commented 5 years ago

spec.js provides a simple config

rewiremock('./foo').withDefault(() => 'fake-foo'); // mock `foo`
index = (await import('./index')).default; // require `index`

Then I am trying to run different scenarios manually renaming files.

everything works when they are js
mocking + require of foo.mjs is working. Ie still goes thought Module._load
mocking + requiring foo.mjs via index.js is working.
mocking + requiring foo.mjs via index.mjs is not working. Look like in case of mjs Module._load is working only for a first file. Later it uses something another.

jdalton commented 5 years ago

Thank you!

Ideally mocking shouldn't work for .mjs at all. What you've uncovered is gaps in our limitation logic to prevent it. So my fixes here would just be making all of the existing places where mocking .mjs works prevented.

theKashey commented 5 years ago

🤯 So - the old ways to mock are dying, but what about new ways? Loaders hooks and loaders maps could handle it, but not in a test-friendly way.

As long as (module-level) dependency mocking is an essential part of any testing - it shall be possible. We don't have any other variant (sinon stubbing is not a variant).

jdalton commented 5 years ago

The .mjs file extension has always lacked esm option support. So these thing that you thought were possible were really never intended for .mjs. Stubbing/spys and mocking work fully with .js.

theKashey commented 5 years ago

Yep. That's why I am asking for advice. Right now it makes .mjs impossible to use. I am quite addicted to dependency mocking, and think that it's a good pattern, and without proper support would have to migrate to more classical DI, which would make everything more classish and complex (as seen in Java).

There should be a way to mock mjs.

jdalton commented 5 years ago

When Node officially supports mocking then you can I guess. For esm you can always use .js. If you insist on .mjs you're likely going to have a bad time across multiple tools beyond esm though.

theKashey commented 5 years ago

Ok, so I have to open issue for a node/modules until it's not too late.

theKashey commented 5 years ago

The problem is tracked elsewhere - https://github.com/nodejs/modules/issues/98

jdalton commented 5 years ago

Thanks @theKashey!

But there's still a consistency issue on our end to track 😸

standard-things / esm