Possible workaround

(function (defineProperty) {
    Object.defineProperty = (obj, prop, desc) => {
        desc.configurable = true;
        return defineProperty(obj, prop, desc);
    };
})(Object.defineProperty);

We’ve ended up writing this small plugin:

function AllowMutateEsmExports() {}

AllowMutateEsmExports.prototype.apply = function(compiler) {
  compiler.hooks.compilation.tap("AllowMutateEsmExports", function(compilation) {
    compilation.mainTemplate.hooks.requireExtensions.tap("AllowMutateEsmExports", source =>
      source.replace("configurable: false", "configurable: true")
    );
  });
};

module.exports = AllowMutateEsmExports;

Then in tests:

    Object.defineProperty(logger, 'createLogger', {
      writable: true,
      value: sinon.stub().returns(loggerStub)
    });

For starters, I vote for this issue to re-open. Webpack 4 breaking nearly every sinon stub test out there when using ESM (the future) doesn’t quite come across to me as a non-issue.

@Janpot

It actually means you’ll need to figure out another way for you to structure your code so that it is both testable and ES6 compliant.

I wonder what sort of structure that might be? In our case we have logger that is used in nearly every class in the system. So should we now inject it all the way from main down to the lowest-level constructs? What guides what gets imported and what gets injected? What needs not and needs stubbing respectively?

And if React is using getContextDispatcher deep down within React.render and all tests triggers use the latter, does that mean that the client API needs to change to include getContextDispatcher only so it can be stubbed in tests?

Or shall we introduce a DI mechanism (which will in effect be just a hack to overcome immutable exports, effectively re-introducing potentially buggy behaviour)?

I accept that Webpack is doing the right thing here - but that is only the right thing in a context-free world. Too much relies on mutable exports at the moment. To much is breaking for too many.

Also, without some solution that does not require grand-refactoring, essentially what people will do is work around the issue by NOT using the right thing - like transpiling to commonjs to run tests. That means if you want tree-shaking (the future) your production build cannot be tested.

It’s a bit like when my company proxy blocks a site, people disconnect from the proxy to access it.

I agree that it would not make sense to “fix” this in Webpack since the same behaviour can be achieved with the Babel loader. Will go ahead and close this issue.

This seems pretty short sighted. What about if you’re not using Babel? This is breaking a lot of our tests in our Typescript projects.

Hello,

We were able to implement @lavelle’s Babel workaround but had to tweak a couple import * as Babel and Webpack handle them differently (happy to provide more details). We could rewrite the code to be testable as @Janpot suggested by exporting a single object vs individual util functions.

It seems weird having to re-introduce CommonJS when we’ve moved everything to ESM. More importantly, it’s too risky to have such variance between our test and production configs. Exporting individual functions helps with tree shaking which we would lose if we exported a single object with functions.

It felt like we were taking a step back with either option so I created a PR that hopefully addresses both: keeps exports immutable by default but allows exports to be configurable.

I spent a lot of time looking into dependency injection libraries (ex. inject-loader, a couple babel plugins…) and they either rely on CommonJS or have shortcomings (inject-loader didn’t respect our babel config). Making exports configurable so getter stubs (like SinonJS) can be leveraged seemed like the most straightforward.

I’d love to hear how the community is handling mocking of ESModules in combination with browser testing. cc @TheLarkInn

Thank you

Update: managed to work around this by telling babel-loader to use the modules: commonjs option in in the env preset. This causes Babel to transpile ES modules into CJS modules before Webpack sees them, which prevents Webpack from creating getters, which fixes Sinon.

https://babeljs.io/docs/plugins/preset-env/#modules

This breaks tree shaking, but we don’t care about tree shaking in unit tests. We can create separate Webpack configs for prod and tests to still get tree shaking in prod.

It may still be worth providing a fix here for people who are not using babel-loader, but for the time being this is no longer blocking our Webpack 4 upgrade.

@fatso83

A few reasons:

• We don’t want to configure two builds using two different tools, especially as these builds are not trivial in nature (typescript, es7, polyfills, pre-processing, you name it).
• The code base is huge, and a build takes around 40 seconds. Webpack already does the smart stuff of only updating what has changed - we don’t want to have another build running building exactly the same thing.
• Our workflow involves running unit tests with every build. So triggering the tests when webpack finishes saves us some sync effort.
• Mind you, there are local builds, ci builds, debug/production builds all sorts of permutations.

Point is, with a simple system what you suggest a lot of sense, but with complex systems there is some sense in a ‘unified’ build approach, to save not only time but the complexity involved.

@Izhaki’s plugin in https://github.com/webpack/webpack/issues/6979#issuecomment-389220417 was working nicely for me as a workaround to this issue, but a recent change in MainTemplate (removal of explicit passing of configurable: false in the module getter definition) highlights that an approach that relies on string replacement is likely to be brittle (and even if parsing was used instead of string replacement, this scenario would require recursive parsing due to the relevant JS living inside a string literal).

I’m wondering whether adding a MainTemplate.hooks.requireHarmonyExportGetter hook for this would be a good solution here. Exposing a hook around this piece of logic should allow for a more stable plugin, and might have lower maintenance overhead than adding an extra option: https://github.com/webpack/webpack/blob/2a9452e51b38b86503ba30a76c65f906b3f99728/lib/MainTemplate.js#L255-L267 (Will PR this approach if there’s support for it or if I get time for POC before then).

@Janpot

I’m largely with you all along, although I suspect you may be mixing state with processes (ie, mutable state with functional dependencies).

What we are talking about here is function a needing function b to fulfil its task (it’s not about data/state). If I take your idea to the extreme, than main should have all the imports and no other script should. This idea is bonkers. What’s more, some of us are writing libraries - we have no main.

I am not saying for a second passing dependencies as parameters is a bad practice - sometimes it is the best design decision; but not always.

So the argument goes:

We need stubs and mocks

These are integral to the development/testing process.

So we need some constructs to be mutable

Now I seriously doubt (and I could be wrong), that our design decisions - what is given as an argument vs what is imported - should be guided by what we need to mock/stub.

The idea that if it needs mocking or stubbing it needs to be provided as an argument is a good guideline, but not always practical (what you mock/stub changes as your project fleshes out). And that’s also related to the whole context headache React is having (and cycle.js).

Immutable exports promote mutability

So with esm exports being immutable, what do we do (look at any idea to overcome the issue in question)? We make what we was immutable, bluntly mutable - and by that open the door for bugs.

configurable: false ?

What I do ask myself is that given (the highly justified) reasons given in the excellent article previously linked, why webpack adds configurable: false?

It seems that it only protects from the property being deleted from the object (which I seriously doubt any code will attempt). Other than that, it just locks the export to be immutable - and that interfere with stubs and mocks.

With configurable: true (and get set), you still get immutable binding export. But at least one that can be later explicitly overridden by stubs and mocks like so:

    Object.defineProperty(logger, 'createLogger', {
      writable: true,
      value: sinon.stub().returns(loggerStub)
    });

Which seems to me the best compromise.

Just fyi, some related topics:

• https://github.com/rollup/rollup/issues/1247
• https://github.com/rollup/rollup/pull/35
• https://github.com/standard-things/esm/issues/266
• https://github.com/benjamn/reify/issues/180
• https://github.com/nodejs/modules/issues/55

I’m still wondering whether this sort of stuff could be implemented as a webpack plugin. Not sure what’s possible, but maybe one that adds some setters? Or even one that plugs in another loader entirely?

@Izhaki Since you asked, usually I inject dependencies using existing standard javascript structures, like function parameters or class constructors. Then create an application root that sets up the whole dependency structure, injects the configuration and starts the main routine. And yes, even the logging mechanism. I’ve long ago started to get away from injecting at the module level as I find that it often introduces more bugs to mutate global state than adopting a more immutable style of programming. i.e. reinstantiate a fresh version the code under test for each testcase. But this a personal experience ofcourse. Honestly, my testing code actually got less buggy when I stopped messing with global state.

We had this problem too, when using:

import * as bar from './index';

in tests.

For us babel-plugin-rewire seems to be a good alternative for this use case.

How far off is a solution on this? Stubbing third party packages makes it impossible to add test coverage to some portions of code.

If I take your idea to the extreme, than main should have all the imports and no other script should.

Indeed, that’s pretty much the idea. In practice you usually wouldn’t take it that far though. It sounds bonkers, but you’d be surprised how decoupled your code becomes. And how easy it becomes to test. When it comes to libraries, this pattern is also usable. Though in most cases the functionality is narrow and easy to test enough to not require it. I can’t stress enough, though. This is a workflow that works for me. It’s based on a relatively substantial amount of experience. But it’s still opinionated and not without tradeoffs.

Thanks, that’s very helpful. I was confused as to why Webpack switched to using Object.defineProperty to create immutable getters, rather than just setting the key on an exports object. Seemed overly complicated. I guess that’s basically just a way to implement the immutable semantics of ES6 modules described in your linked article?

Sounds like there’s a couple of issues here that were both triggered at once for us:

1. Sinon’s stub and spy only work with regular object methods, not getters, and so it’s not compatible with the new Webpack export system.
2. Sinon inherently relies on mutable exports, since you’re stubbing out a method in one file (your unit test case file) and expecting that change to be reflected in another file (the file under test).

It seems like using the Babel transform to enable CJS semantics only for unit tests is the right way forward here then. We get ES6 semantics and all the tree-shaking benefits in our prod build, but still enable mutable exports in unit tests to allow Sinon to work.

I agree that it would not make sense to “fix” this in Webpack since the same behaviour can be achieved with the Babel loader. Will go ahead and close this issue.

@lavelle The old behavior is wrong. This should explain it fairly well: http://2ality.com/2015/07/es6-module-exports.html . Personally I don’t think it makes sense to add a configuration to revert to this buggy behavior. It actually means you’ll need to figure out another way for you to structure your code so that it is both testable and ES6 compliant.

For context, we are running into an issue with Sinon that only started happening when we upgraded to Webpack 4. See details on that here: https://github.com/sinonjs/sinon/issues/1762

Webpack 4 works fine for our production build, we just need a way to get it to work with Sinon to fix our unit tests and unblock the upgrade.