Skip to content

fix(package): specify side-effects in package.json #383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mayorandrew
Copy link

Description:

This prevents bundlers from tree-shaking Web Streams API compatibility extensions.

Related issue (if exists):

#382

Comment on lines 23 to 47
const sideEffects = [
"./asynciterable/todomstream.mjs",
"./asynciterable/todomstream.js"
];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this change, the prototype patching needs to be captured. We should probably make this recursively read and add all the files in src/add, since prototype patching is their entire purpose.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted the implementation to also dynamically add all files from src/add here.

I just used glob for this. Please let me know if that is good enough from the perspective of integrating with gulp.

@trxcllnt
Copy link
Member

trxcllnt commented May 5, 2025

Inspecting the yarn test:bundle rollup output for toDOMStream, I see the pipeThrough function on the prototype:

image

Do you see anything missing in our treeshaking integration tests rollup configuration?

@mayorandrew mayorandrew force-pushed the package-json-side-effects branch 2 times, most recently from 687cca8 to ae80690 Compare May 11, 2025 10:25
This prevents bundlers from tree-shaking Web Streams API compatibility extensions.

Closes ReactiveX#382
@mayorandrew mayorandrew force-pushed the package-json-side-effects branch from ae80690 to d2a2586 Compare May 11, 2025 10:26
@mayorandrew
Copy link
Author

Inspecting the yarn test:bundle rollup output for toDOMStream, I see the pipeThrough function on the prototype:
Do you see anything missing in our treeshaking integration tests rollup configuration?

@trxcllnt, I'm not sure if you ran this on my PR branch or on master, and I also don't see the full path to the file on the screenshot, to fully understand the concern.

Running tests

I've run the yarn test:bundle locally, and the only files I see under integration/rollup/ix are asynciterable.js and iterable.js which represent the bundle output of the corresponding integration tests. Neither includes pipeThrough or toDOMStream even on my PR branch.

This affects (rollup|webpack)/(es5|es2025|esnext)/(esm|umd) and (rollup|webpack)/ix (basically, everything except cjs).

Screenshot 2025-05-11 at 12 35 35

Test with side-effect import

I have created a new test locallly, asynciterable.dom.js which looks like this:

import 'ix/asynciterable/todomstream';
import { AsyncIterableX } from 'ix/asynciterable';
console.log(AsyncIterableX.prototype.toDOMStream);

As you can see, I added a side-effect import there: import 'ix/asynciterable/todomstream';. Without this, the toDOMStream function definition is not included in the bundle even on my branch.

This seems to be related to the fact that the original test only uses the AsyncIterableX symbol, not other export, so rollup and webpack agressively tree-shake the todomstream.js file even though there is a re-export in asynciterable/index.ts.

Test with the main entrypoint

Since the original test imports directly from 'ix/asynciterable', I think the dom.js entrypoint is bypassed. I've tried to also test what happens if we let rollup use it, by changing the test to:

import { AsyncIterable } from 'ix';
console.log(AsyncIterable.prototype.toDOMStream);

This doesn't work either. Even if I explicitly add import 'ix'; or import 'ix/dom'; to the test, and import './asynciterable/todomstream.js'; to dom.ts, rollup just tree-shakes it away anyway even on my PR branch.

Test with explicit import of toDOMStream operator

Since ix/asynciterable re-exports toDOMStream operator, I also tested whether the prototype method is added if I explicitly import that operator.

import { AsyncIterableX, toDOMStream } from 'ix/asynciterable';
console.log(toDOMStream);
console.log(AsyncIterableX.prototype.toDOMStream);
console.log(AsyncIterableX.prototype.pipeThrough);

This setup results in the AsyncIterableX.prototype.toDOMStream and AsyncIterableX.prototype.pipeThrough functions being included in the bundle even on the master branch.

However, this dependency is obscure, and the library could still benefit from allowing the explicit side-effect import or presreving the side-effect automatically when the browser environment is used.


Please let me know your thoughts on what would be the best way to proceed with this.

@trxcllnt
Copy link
Member

trxcllnt commented May 13, 2025

I ran yarn test:bundle on master just to double check. If you run ./integration/make-files-to-bundle.sh, it will generate source files that import each operator (these are not checked in).

The generated integration/src/asynciterable/toDOMStream.js file looks like this:

import { toDOMStream } from 'ix/asynciterable';
console.log(toDOMStream);

This seems to match your last test where the operator is imported and then used in a way where tree-shaking won't discard it. This is what I'd expect from tree-shaking -- it elides code that isn't directly imported and used.

But from your description in #382, it sounds like you're importing toDOMStream, but instead of relying on it directly in your application code, you're relying on AsyncIterableX.prototype.pipeThrough prototype method that happens to use toDOMStream. Since rollup's tree-shaking doesn't detect this as using toDOMStream in your code, it elides the module.

This makes me wonder if we shouldn't move the pipeTo, pipeThrough, and toDOMStream prototype methods to src/add/asynciterable, then they can be imported directly and marked as having side-effects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants