GH-38936: [JS] initialize overrides for DOM and Node in IIFE

[domoritz]

• Closes: [JS] The package.json file incorrectly specifies sideEffects: false #38936

[github-actions]

⚠️ GitHub issue #38936 has been automatically assigned in GitHub to PR creator.

[domoritz]

@nrabinowitz can you test this change? If we get it in my Monday, it will be in the next release.

[trxcllnt]

@domoritz I think the IIFE here works because it's initializing a static property on the class, so bundlers can't throw away the side effects. I think we'd have to do something similar here.

[domoritz]

I see. The issue with my code is that the method might still be thrown away?

[trxcllnt]

@domoritz yes, I believe so. I'm not sure if there's a clean way to do this...

[trxcllnt]

What impact would setting sideEffects: true have?

[domoritz]

My understanding is that it makes tree shaking much less effective but we should check to see the impact. Or is there some way to remove the side effect for example by extending the builder and batch reader base classes with the node and dom specific methods and then exporting an extended class?

[trxcllnt]

extending the builder and batch reader base classes with the node and dom specific methods and then exporting an extended class

The internal factory functions that create builders will be creating instances of the non-extended class, but we could definitely use this strategy as a way to patch the prototypes w/ the static-property-initialized-by-an-IIFE style that defeats tree-shaking.

[nrabinowitz]

@nrabinowitz can you test this change? If we get it in my Monday, it will be in the next release.

Sorry I missed this until now. Based on the discussion here, do you still want me to test this fix as it stands or should I wait for another approach?

FWIW, my current workaround is to use a Webpack override to specify sideEffects: true. This works, but I agree with the assessment that tree-shaking will not be as effective (and may not be used at all).

[domoritz]

Based on the discussion here, do you still want me to test this fix as it stands or should I wait for another approach?

I'm curious whether it works or not.

The internal factory functions that create builders will be creating instances of the non-extended class, but we could definitely use this strategy as a way to patch the prototypes w/ the static-property-initialized-by-an-IIFE style that defeats tree-shaking.

How could that work? If someone imports only makeBuilder or builderThroughIterable for example, they might not import Builder and so we wouldn't execute the