Bug: `react-dom-webpack-server` bundles are currently broken on `main`

[markerikson]

React version: N/A (Build tooling issue on main as of commit 0131d0c )

Steps To Reproduce

1. Clone, install, and run yarn build
2. Inspect build/oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.development.js
3. Scroll to line 300, around var reservedProps =. Compare with react-dom-bindings/src/shared/DOMProperty.js. Note that:

• There is no properties object in the build output
• The lines that should say properties[name] = new PropertyInfoRecord(....) are missing the assignment, and only say new PropertyInfoRecord()
• A couple of the associated constants like const RESERVED = 0 are also missing

This appears to be a bug in Rollup's tree-shaking behavior. If the Rollup config in build.js is modified to say moduleSideEffects: 'safest', the missing content appears in these bundles.

Per experimentation, this can be addressed by adding a custom Rollup plugin that specifically tells Rollup "this code has side effects, don't tree-shake it" (based on rollup/rollup#4090 (comment) ):

module.exports = function disableTreeshake() {
  return {
    name: 'scripts/rollup/plugins/disable-treeshake',
    transform(code, id) {
      // Fix issue with `react-dom-webpack-server` bundles accidentally
      // stripping out the `properties` object and not filling it out
      if (id.endsWith('DOMProperty.js')) {
        return {
          code,
          map: null,
          moduleSideEffects: 'no-treeshake',
        };
      }
      return null;
    },
  };
};

Link to code example:

The current behavior

The full code from DOMProperty.js does not show up in those react-dom-webpack-server bundles

The expected behavior

The DOMProperty output should show up.

[gaearon]

Do we actually know that it should show up? Is there an observable difference in behavior, or does Rollup correctly eliminate unused code?

[markerikson]

I haven't tried to trace the usage of that code in the react-dom-webpack-server bundles.

If I look at what seems to be an equivalent section of one of the prod bundles, I see:

"allowFullScreen async autoFocus autoPlay controls default defer disabled disablePictureInPicture disableRemotePlayback formNoValidate hidden loop noModule noValidate open playsInline readOnly required reversed scoped seamless itemScope".split(" ").forEach(
  function(a){new y(a,3,!1,a.toLowerCase(),null,!1,!1)
})
;["checked","multiple","muted","selected"].forEach(
  function(a){new y(a,3,!0,a,null,!1,!1)
});

Which looks like it is that same code, with no assignment statement (ie, the new y() corresponds to new PropertyInfoRecord(), and that value is currently being created and thrown away).

[gaearon]

Yeah but the question is whether that properties object is being used in them. If it's not being used, isn't it correct that it gets removed?

[markerikson]

Here's a copy of the "fixed" dev bundle from one of my local builds:

react-server-dom-webpack-server.browser.development-fixed.zip

Looking at it, the properties object is:

• Written to by all of the properties[name] = new PropertyInfoRecord() lines
• Read only by getPropertyInfo()

and I don't see any other references to getPropertyInfo() in that bundle file.

So, it does seem like all of the DOMProperty.js logic may actually be dead as far as this bundle is concerned. (Although in that case, you'd think that all of it would get stripped out, especially in a prod build.)

Given that, this may not actually be a true bug in terms of behavior.

Not sure what the right answer here is, then. I guess adding an extra plugin just to fix the output isn't necessary if it's not going to result in a difference in behavior, but it also seems odd to have seemingly-incorrect bundle output and leave that extra dead logic in the bundles 🤷‍♂️

[gaearon]

Is the output incorrect or insufficiently optimized?

If it's not being stripped out, that's something that would be good to fix, but it's not the same as it being broken. react-server-dom-webpack-server bundle should usually be Node-only, so this shouldn't be a big problem re: code size either.

The problematic part here might be convoluted metaprogramming we do in DOMProperty but we're moving towards getting rid of it in #26433.

[markerikson]

Yeah, after inspecting the bundles, I would say that it's just that they're "insufficiently optimized". It does seem like there's a Rollup bug in that it should either leave in all of DOMProperty.js or strip all of it out, and not strip out a few specific parts. But, given that the properties stuff doesn't seem to be used in those bundles, I think it's safe to leave that alone for now.

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[github-actions]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!