Transpilation in v16.1.0 freezes the react-dom/server interface

[travi]

As initially reported in #11520 (comment), attempting to upgrade from v16.0 to v16.1 caused a test that was previously working to fail. The test was using sinon to stub renderToStaticMarkup on react-dom/server and v16.1 results in the following error:

TypeError: Cannot redefine property: renderToStaticMarkup
    at Function.defineProperty (<anonymous>)
    at wrapMethod (node_modules/sinon/lib/sinon/util/core/wrap-method.js:92:16)
    at stub (node_modules/sinon/lib/sinon/stub.js:60:44)
    at Object.stub (node_modules/sinon/lib/sinon/collection.js:102:33)
    at Context.setup (test/unit/....)

the export from react-dom/cjs/react-dom-server.node.development is frozen as:

Object.freeze({
  renderToString: renderToString,
  renderToStaticMarkup: renderToStaticMarkup,
  renderToNodeStream: renderToNodeStream,
  renderToStaticNodeStream: renderToStaticNodeStream,
  version: version
});

the source is not frozen, and #11520 (comment) further confirms that the Object.freeze() is a result of the transpilation.

for now, i am simply splitting this report into a separate issue from #11520, but i will add a reproduction soon and plan to submit a PR as a follow up.

[travi]

a reproduction is available at https://github.com/travi/react-dom-frozen

[gaearon]

Here is how to fix it: #11520 (comment)

Happy to take a PR for this.

[travi]

I hope to put a PR together for this sometime today/tonight. I just ran out of time last night

[gaearon]

So, I'm not super happy with this solution, but it seems easier to do this now and maybe break this in 17.

Longer term I would encourage you to use mocking solutions that don't require monkeypatching public APIs, if possible. This would break when Node ships ES modules and React offers an ES module version anyway.

[travi]

Thats a very valid point, but I really appreciate the understanding to keep this working for the time being.

As a testing advocate, this is what caught my eye about the static nature of es module imports early on and I'm really surprised that I haven't seen more concern about it from the js testing community (likely because tools like babel have insulated things enough that it hasn't been forced yet). Not that it is wrong in any way, but it will require pretty drastic changes to testing approaches in js projects to keep them testable. I expect dependency injection and app-level coordination along the lines of an IoC container to become required, but I haven't seem momentum behind such solutions yet, outside of angular.

While it is nice to see the ecosystem evolve and mature, it is a little bittersweet to see the current simplicity and approachability to testing in js headed toward significantly more complexity. I hope that some reasonable solutions emerge by the time React and others start publishing versions that cause current approaches to no longer be valid.

Thanks again for the understanding and helping this get resolved for the time being. My first contribution to React has been pretty painless as well. Thanks for all you do!

[gaearon]

I expect dependency injection and app-level coordination along the lines of an IoC container to become required, but I haven't seem momentum behind such solutions yet, outside of angular.

In case of React, people commonly use Jest which implements its own module system and can jest.mock() anything. I guess that more or less helps with this problem.

But I agree I expect more people to raise concerns over this with time.

[gaearon]

The fix should be out in 16.1.1.

[travi]

implements its own module system and can jest.mock() anything

interesting. i have not tried jest since the days when it was really slow since i've been pretty happy with the combination of mocha, chai, jsdom, enzyme, and sinon, but i have not found a solution in that mix for handling truly static imports yet.

i've been holding off on jest for the time being until i found a feature that truly set it apart. managing its own module system to keep the static imports testable might just be that feature. thanks a ton for following up and mentioning it. i had not heard about that yet, so i'll have to investigate.