Treeshaking is not working

[frehner]

While working on #1060, I've discovered that treeshaking isn't working and we're pulling in everything when we do a build. ☹️

I've done a bit of digging by adding the following to the top of our plugins list at plugins.ts:

{
  name: 'testing',
  moduleParsed(info) {
    if (info.id.includes('hydrogen-ui')) {
      console.log('---------------------------------');
      console.log(info.id, info.hasModuleSideEffects);
      console.log('---------------------------------');
    }
  },
},

and here's an example of the output from that:

template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: /Users/af/dev/hydrogen/packages/hydrogen-ui/dist/index.server.js false
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: /Users/af/dev/hydrogen/packages/hydrogen-ui/dist/index.client.js false
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: /Users/af/dev/hydrogen/packages/hydrogen-ui/dist/jsx-runtime.js false
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: /Users/af/dev/hydrogen/packages/hydrogen-ui/dist/index.client.js?no-proxy true
template-hydrogen-default:build: ---------------------------------

The actual source files are being correctly marked as not having sideEffects, but the client component proxy that we create in the RSC Vite plugin is saying that it does has side effects. (Rollup won't treeshake files that have side effects)

Some things I've found while researching this:

Client proxy source file has side effects

Looking at the client-proxy source code, I think it's fair to say that it does have sideEffects

Client proxy module could help give hints about side effects

The way we create the proxy component could maybe help give hints to Rollup that it can be treeshaked better, too;

1. We could return an object from proxyClientComponent instead of just the code, since we're in the load hook. That object could look like return {code: proxyCode, moduleSideEffects: false}
2. In testing, it seems that that wasn't enough to convince Rollup that the proxy module didn't have side effects; then I looked at what the module code looked like and played with it in the Rollup repl, and found that the way we have written the module doesn't mesh well with Rollup's treeshaking - notice how function B is still in the output, even though it's not being used in the entry module. However, I then copied the little trick that React/JSX does and put a magic comment before the call, and now function B is no longer found in the output. So we could probably do the same thing in our proxy module?

By doing these two things, I was able to convince Rollup that the proxy module didn't have sideEffects, as seen with the output

template-hydrogen-default:build: ---------------------------------
template-hydrogen-default:build: /Users/af/dev/hydrogen/packages/hydrogen-ui/dist/index.client.js?no-proxy false
template-hydrogen-default:build: ---------------------------------

And yet, the client components were still in the output bundles.

So it could be that rollup realizes that we do actually have side effects (as noted in the section "Client proxy source file has side effects"), or that there's still additional work to do here.

Or maybe I'm just completely wrong about all of this. Haha. 🙂

[frehner]

Oh, another thing I did was to add the following code to the resolveId hook

if(source.endsWith('?no-proxy')) {
  return {
    id: source,
    moduleSideEffects: false
  }
}

[developit]

The component wrapping can be elided automatically by Rollup if invocations are preceded by a Terser-style /*@__PURE__*/ annotation:

export const A = /*@__PURE__*/Test(mod2.A)
export const B = /*@__PURE__*/Test(mod2.B)

(view in Rollup REPL)

[frehner]

Yeah, that's great! I think I also found that in bullet 2 above. 💯

In testing, it seems that that wasn't enough to convince Rollup that the proxy module didn't have side effects; then I looked at what the module code looked like and played with it in the Rollup repl, and found that the way we have written the module doesn't mesh well with Rollup's treeshaking - notice how function B is still in the output, even though it's not being used in the entry module. However, I then copied the little trick that React/JSX does and put a magic comment before the call, and now function B is no longer found in the output. So we could probably do the same thing in our proxy module?

[developit]

aughh, sorry, I jumped into the Rollup REPL too quickly and missed that whole blurb ☠️

[benjaminsehl]

@frehner — is this still relevant?

[frehner]

Yup, though it appears I somehow missed the related PR that @frandiox put up!

Could do a little testing to see if it's working now or not.