`@sentry/core` is marked as being side effect free but it is not

[timfish]

Description

sentry-javascript/packages/core/package.json

Line 65 in b746c23

"sideEffects": false,

Our build output contains imports where Rollup detects side effects:

But time.ts is the most obvious place:

sentry-javascript/packages/core/src/utils/time.ts

Line 67 in b746c23

export const timestampInSeconds = createUnixTimestampInSecondsFunc();

Our current build output gives warning with Cloudflare's Wranger:

▲ [WARNING] Ignoring this import because "../../../../packages/core/build/esm/utils/time.js" was marked as having no side effects [ignored-bare-import]

 ../../../../packages/core/build/esm/trpc.js:8:7:
 8 │ import './utils/time.js';
 ╵ ~~~~~~~~~~~~~~~~~

 "sideEffects" is false in the enclosing "package.json" file:

 ../../../../packages/core/build/esm/package.json:1:36:
 1 │ {"type":"module","version":"9.35.0","sideEffects":false}
 ╵ ~~~~~~~~~~~~~

[mydea]

 export const timestampInSeconds = createUnixTimestampInSecondsFunc(); 

Is that considered a side effect? 🤔 this exports a function, which I would have expected to be OK...?

[timfish]

Unfortunately Rollup it's that clever. It considers any execution that occurs from only importing a file as a side-effect.

It even detects this as a side effect:

sentry-javascript/packages/core/src/tracing/errors.ts

Lines 41 to 43 in ceb003c

	// The function name will be lost when bundling but we need to be able to identify this listener later to maintain the
	// node.js default exit behaviour
	errorCallback.tag = 'sentry_tracingErrorCallback';

That can be fixed quite easily by moving the callback definition inside registerSpanErrorInstrumentation.

Rollup has an experimentalLogSideEffects option which we could enable to highlight issues. We could also use treeshake.moduleSideEffects to tell Rollup we don't have any side effects, or to disable it for specific files we know to be safe.

[mydea]

hmm, that setting experimentalLogSideEffects seems a bit overly strict. it also flags e.g. debug-build.ts which is export const DEBUG_BUILD = __DEBUG_BUILD__; 😅

How would we use treeshake.moduleSideEffects ? 🤔

[timfish]

experimentalLogSideEffects seems safe to use because I think it just logs when it finds side-effects.

If you set treeshake.moduleSideEffects: false Rollup assumes there are no side effects anywhere. Probably not always safe but if the tests are all great and pass then it's probably ok.

You can also set it as a callback and override Rollups side-effect detection on a per-module basis.

[mydea]

I think I have opened PRs for all side effects in core, except for debug-build... no idea how to get rid of the warning there, this seems fine to me 😅

declare const __DEBUG_BUILD__: boolean;

/**
 * This serves as a build time flag that will be true by default, but false in non-debug builds or if users replace `__SENTRY_DEBUG__` in their generated code.
 *
 * ATTENTION: This constant must never cross package boundaries (i.e. be exported) to guarantee that it can be used for tree shaking.
 */
export const DEBUG_BUILD = __DEBUG_BUILD__;

[timfish]

except for debug-build... no idea how to get rid of the warning there

I think its because the side-effect analysis happens before the define plugin runs so it looks like DEBUG_BUILD is being set from a __DEBUG_BUILD__ global.

We might be able to use treeshake.moduleSideEffects to override side-effects for all ./debug-build.ts files. I'll check it out