feat: Allow to configure `bundleSizeOptimizations` #428

mydea · 2023-10-19T12:40:47Z

This introduces a new option for the plugins:

sentryPlugin({
   bundleSizeOptimizations: {
      excludeDebugStatements: true,
      excludePerformanceMonitoring: true,
      excludeReplayCanvas: true,
      excludeReplayIframe: true,
      excludeReplayShadowDom: true,
    },
});

When set, this will replace the relevant build time flags with a boolean, allow bundlers to treeshake code out and reduce the shipped bundle size.

Closes #425

Lms24

I like the API for this! Probably easier to use for the majority of folks than defining a replace plugin themselves.

Do you think there's a way that we could filter out files before reading them? I'm thinking about looking at the file path and checking for a @sentry occurance. But not sure if I'm missing something tough why this wouldn't work.

Lms24 · 2023-10-19T12:52:56Z

packages/bundler-plugin-core/src/index.ts

+    renderChunk(code: string) {
+      return replaceBooleanFlagsInCode(code, values);
+    },
+
+    transform(code: string) {
+      return replaceBooleanFlagsInCode(code, values);
+    },
+  };


Why do we need to call the replace function in two hooks? Shouldn't transform be enough?

Tbh I just copied this behaviour from the rollup replace Plugin 😅 they to it that way there. But I can just remove the chunk hook!

lforst · 2023-10-19T12:53:06Z

packages/esbuild-plugin/src/index.ts

+
+          const contents = replaceBooleanFlagsInCode(source, values);
+
+          if (!contents) {
+            return null;
+          }
+
+          return { contents: contents.code, loader: "default" };
+        });
+      },


We cannot do this as it will override any other registered laoder for files that match this filter. Let's do the following instead:

setup({ initialOptions }) { initialOptions.define = { ...initialOptions.define, ...values }; },

just to be clear, as I am not an expert at this at all - we do not do this for the other esbuild plugins, which is where I basically copied this from (as well as looking at some esbuild replace plugin). or is this only because we match all files here? E.g. here: https://esbuild.github.io/plugins/#using-plugins they also just return this directly...?

In the other plugins, we are doing very complex logic to make sure all of the user files are still loaded via the plugins they defined. There we are using a sort of proxy logic.

The onLoad hook is simply the wrong approach for this. The onLoad hook, as it suggests is for "reading" files off a particular location. For example, esbuild doesn't know how to files that lie somewhere on the internet (http) so you can define an onLoad hook that takes care of that for esbuild. A file can only be loaded once. So if our plugin loads it, no other plugin will be able to. Doing this in our case will result in catastrophic behavior for users that use any kind of loader plugin - in the way it is implemented right now this will pretty much result in catastrophic behavior for ALL users since we always define the load hook.

Esbuild doesn't have any sort of transform hook and that is by design to keep it fast. It does all the transpilation and stuff by itself. We need to use the define option.

Ah, that makes sense, thank you for the in depth explanation! I'll adjust this then according to this 🙏

I updated this to use define (which is much nicer anyhow!)

lforst · 2023-10-19T12:57:50Z

packages/bundler-plugin-core/src/index.ts

+  return {
+    renderChunk(code: string) {
+      return replaceBooleanFlagsInCode(code, values);
+    },
+
+    transform(code: string) {
+      return replaceBooleanFlagsInCode(code, values);
+    },
+  };


We have a slight issue but I don't know yet whether it is gonna be a problem.

We generally instruct people to put our plugin as late as possible, this is necessary for debugID injection. However, in order for minification plugins to be able to properly tree shake the dead code we introduce here, we need to be as early as possible...

I think this is fine with the transform hook, assuming that it is early enough. The render chunk hook is probably too late. I think we can just remove the render chunk hook in general. If we don't catch the statements in transform, we're too late anyhow.

I will try removing renderChunk and see how it goes. we can always revisit this and add it again later!

lforst · 2023-10-19T13:02:21Z

packages/bundler-plugin-core/src/utils.ts

+    }
+  });
+
+  if (ms.hasChanged()) {


this is nice!

packages/bundler-plugin-core/src/utils.ts

packages/bundler-plugin-core/src/index.ts

lforst · 2023-10-19T13:04:53Z

packages/webpack-plugin/src/index.ts

+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore webpack version compatibility shenanigans
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+      const DefinePlugin =
+        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+        // @ts-ignore webpack version compatibility shenanigans
+        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+        compiler?.webpack?.DefinePlugin ||
+        webback4or5?.DefinePlugin ||
+        webback4or5?.default?.DefinePlugin;


beautiful :D

lforst · 2023-10-19T13:13:42Z

Do you think there's a way that we could filter out files before reading them? I'm thinking about looking at the file path and checking for a @sentry occurance. But not sure if I'm missing something tough why this wouldn't work.

Not a bad idea. we could do id.contains(os.sep + "node_modules" + os.sep + "@sentry" + os.sep). This has a few edge cases with resolution but in 99% of the cases it should work.

Counterargument, it would not be the same behavior for all bundlers and why should we make this unnecessarily fragile?

Lms24 · 2023-10-19T13:51:04Z

Counterargument, it would not be the same behavior for all bundlers and why should we make this unnecessarily fragile?

Also a good point. My concern is mostly that the plugin having to read every file might have a performance impact. But I'm totally fine if we just go with this first and optimize later if necessary.

mydea · 2023-10-19T14:46:34Z

Do you think there's a way that we could filter out files before reading them? I'm thinking about looking at the file path and checking for a @sentry occurance. But not sure if I'm missing something tough why this wouldn't work.

Yeah we can for sure revisit this, since this is opt in I think we can tweak this more later if needed!

mydea · 2023-10-20T09:19:29Z

OK so I fixed the tests and tried to address the feedback - is this ready to go? :)

I did not add any file filtering for now, I think we can still add this at a later stage if we find out this is too slow! This does not run at all if no bundleSizeOptimizations are configured so it should not affect any existing users.

lforst · 2023-10-20T10:13:32Z

This concern is critical and we need to address it before merging this.

lforst · 2023-10-23T09:14:34Z

packages/esbuild-plugin/src/index.ts

+      setup({ initialOptions }) {
+        const replacementStringValues: Record<string, string> = Object.entries(
+          replacementValues
+        ).reduce((acc, [key, value]) => {
+          return { ...acc, [key]: JSON.stringify(value) };
+        }, {});


this is an accidental O(n^2) because of the spread in a loop, I don't think it matters a lot but since this is an easy fix we should just fix it.

mydea requested review from Lms24 and lforst October 19, 2023 12:40

mydea self-assigned this Oct 19, 2023

feat: Allow to configure bundleSizeOptimizations

c87d6f9

mydea force-pushed the fn/bundleSizeOptimizations branch from 5c786dc to c87d6f9 Compare October 19, 2023 12:43

Lms24 reviewed Oct 19, 2023

View reviewed changes

lforst reviewed Oct 19, 2023

View reviewed changes

mydea added 4 commits October 20, 2023 08:48

PR feedback

e435c0d

fix whitespace tests

b08f9ff

fix expected checks

efa198e

make bundle size optimization tests less flaky

a653af5

fix esbuild plugin

2b593f3

lforst reviewed Oct 23, 2023

View reviewed changes

lforst approved these changes Oct 23, 2023

View reviewed changes

minor perf refactor

8df41d8

mydea merged commit 2b75688 into main Oct 23, 2023

mydea deleted the fn/bundleSizeOptimizations branch October 23, 2023 10:20

lforst mentioned this pull request Jan 30, 2024

feat(core): Automatically annotate React components at build time #464

Closed

feat: Allow to configure bundleSizeOptimizations #428

feat: Allow to configure bundleSizeOptimizations #428

Uh oh!

Conversation

mydea commented Oct 19, 2023

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lforst commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lms24 commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mydea commented Oct 19, 2023

Uh oh!

mydea commented Oct 20, 2023

Uh oh!

lforst commented Oct 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: Allow to configure `bundleSizeOptimizations` #428

feat: Allow to configure `bundleSizeOptimizations` #428

lforst commented Oct 19, 2023 •

edited

Loading

Lms24 commented Oct 19, 2023 •

edited

Loading