`@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.