Added output.moduleExportsConfigurable for mocking exports in tests

[gmathieu]

What kind of change does this PR introduce?

Added output.moduleExportsConfigurable that defines export properties as configurable so es6 exports can be mocked.
Mocking exports only works when mode != 'production'.

Did you add tests for your changes?

Yes

If relevant, link to documentation update:

If this PR is accepted, I'll happily create a PR in the docs repo.

Summary

Continuing off this issue...

Before Webpack 4 certain ES Module exports were assigned to the export object by mutating it.
Most stubbing libraries like SinonJS rely on the fact that objects are mutable to apply stubs, spies, etc...
I understand mutating exports can have undesirable consequences and Webpack 4 did a great job fixing this.
This PR keeps the default behavior intact and adds a configuration to enable simple mocks bypassing complex dependency injection libraries.

Does this PR introduce a breaking change?

No

Other information

I'm not married to the name of "moduleExportsConfigurable" and happy welcome feedback on only allowing this when mode === development.

Stubbing getter attributes with Sinon.JS: http://sinonjs.org/releases/v4.5.0/stubs/#stubgetgetterfn

[jsf-clabot]

All committers have signed the CLA.

[webpack-bot]

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

[gmathieu]

@sokra any thoughts on this?

[gmathieu]

@TheLarkInn what do you think of allowing this? This is the last outstanding item before we can upgrade to Webpack 4 (which is considerably faster for the size of our codebase). Thanks in advance.

[Janpot]

@gmathieu To me it seems like this use-case is broader than just webpack and my feeling is that it should be solved in collaboration with the other players in the ecosystem.
I know for instance that the node modules team has added it to their use-cases.
Do you think this functionality can be solved by writing a custom loader instead?

[gmathieu]

@Janpot I agree and I'm glad that the node modules team looking into this (there's clearly a need there). It seems both @bregenspan and I have tried custom loaders with no luck (#6979 (comment) and #6979 (comment)).

Given the mess that is dependency injection in the JS world, I would imagine we're not the only ones who exploited mutable exports in previous Webpack versions. I understand this is not ideal or a permanent solution (at least until the community solves this issue which could be a while). It does remove a potential blocker for people upgrading to Webpack 4 and future versions which seems like a good thing since loaders are only getting better as Webpack evolves (and new loaders don't work on older versions).

[Janpot]

Ok, but isn't inject-loader just one possible implementation that is just too limited for your use-case? (or perhaps it's just a bug on their side?)

[gmathieu]

Most definitely. The way I see it, there is two types of loaders both of which sound limiting:

1. a loader like inject-loader that needs to understand how to parse an import to rewrite it which means adding support for any babel version and config. After that, developers have to reorganize their code because only direct dependency can be stubbed
2. going down the path of ESM -> CJS -> dependency injection library (ex. rewire) which creates an even bigger gap between our test config and production. Developers have to then convert existing stubs with the new mocking library therefore adding more domain knowledge.

Webpack 4 has done an incredible job at standardizing all different types of exports into a consistent format. I feel like it's the most straightforward place to apply a stub but I could be wrong.

@Janpot are you afraid this would lock Webpack into a specific way of organizing modules preventing further change down the road?

[Janpot]

No, my fear is that this use-case is only doable if you can hook into the runtime module loader. It might just be that putting runtime loader hooks in webpack isn't a great idea (or even possible). Which means that the hack you're trying to squeeze in here may not be as temporary as it seems.

I just don't think, just because nobody has come up with the right idea yet, that any hack is good enough for the time being. Especially not toggles that create non-standard behavior that people will start to rely on and which will become technical debt and a breaking change to remove.

[bregenspan]

@Janpot the way I see this is that a number of teams (including mine, so there is definitely a lot of self-interest in this argument) were already relying on the non-standard behavior, which at least at the time was an elegant way of achieving the goal of runtime stubbing. Moving to be spec-compliant is correct but at the same time a breaking change (to unspecified but seemingly widely relied-on behavior*). Based on this, there could (depending on the philosophy of the Webpack team) be an argument for incurring technical debt/implementing some "non-strict mode" for this case if it turns out that it smooths the upgrade path for a significant number of codebases. I personally like the approach in this PR but am obviously biased. My team will probably work around this for now by doing some rearchitecting of the dependencies we had been stubbing/spying on to make them explicitly support runtime stubbing, which might or not be practical for other teams.

_{* (I'm basing this mostly on common answers to this question on Stackoverflow, which is obviously not always the best place to find the "correct" answer but can be a good place to survey what people are doing in practice.)}

[Janpot]

@bregenspan Just for the record, I maintain code affected by this issue as well. 🙂

[maxkostow]

Since configurable: false is a breaking change anyway, I think unbreaking it now and pushing the breaking change further down the road until we can find a better runtime import hook is a reasonable solution. If you follow that logic, the only thing that should block this PR is if @sokra and team are planning to rewrite the internal module system in a way that would break this method anyway.

[Izhaki]

This PR and the issue behind it need immediate addressing in my view: #6979 (comment)

[webpack-bot]

For maintainers only:

• This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

[Izhaki]

Facing the very same issue, 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)
    });

[gmathieu]

Thanks @Izhaki

[Izhaki]

It would be interesting to get some feedback to the last section in this comment

It argues that Webpack should set configurable: true rather than configurable: false for esm imports.

[jdalton]

@gmathieu

I'm interested in enabling better mocking support in esm loader. While I was aware of a request I didn't know the demand was so high for mocking namespace objects (ns of import * as ns from "mod").

If you or others affected would like to head over and bikeshed options it'd be super helpful for making sure your use case is covered.

[mhuggins]

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

[torahmike]

Why wasn't this merged?