Skip to content
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

chore: mark packages as side effects free for tree shaking #3087

Closed
wants to merge 1 commit into from

Conversation

ogxd
Copy link

@ogxd ogxd commented Jul 11, 2022

Which problem is this PR solving?

Fixes #2855

For instance, when using the exporter-trace-otlp-http package for a simple create/send trace, having sideEffects set to false for all packages it depends on results in a 70kb smaller bundle (minified)

Short description of the changes

Marked all package as side-effect free with "sideEffects": false.
This is supposed to be safe:

We talked about this in the SIG meeting and agreed that this is safe for us. Even our modules which have side-effects do not execute those side-effects at load time and require a user to explicitly call a function. This will cause the static analysis of webpack to realize the module is used and not tree-shake it out.
@dyladan

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change could benefit from a mention in the documentation

How Has This Been Tested?

Tested manually using samples and setting sideEffects: false manually in node_modules

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@ogxd ogxd requested a review from a team July 11, 2022 20:44
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@dyladan dyladan marked this pull request as draft July 11, 2022 20:45
@dyladan
Copy link
Member

dyladan commented Jul 11, 2022

I am going to mark this as a draft until the CLA is signed. Feel free to mark it as ready when the checks are passing and it is ready for review.

@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #3087 (bccd2ad) into main (b4707e4) will decrease coverage by 0.41%.
The diff coverage is n/a.

❗ Current head bccd2ad differs from pull request most recent head 54f7240. Consider uploading reports for the commit 54f7240 to get more accurate results

@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
- Coverage   93.08%   92.66%   -0.42%     
==========================================
  Files         188      173      -15     
  Lines        6261     5523     -738     
  Branches     1323     1175     -148     
==========================================
- Hits         5828     5118     -710     
+ Misses        433      405      -28     
Impacted Files Coverage Δ
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...kages/opentelemetry-sdk-trace-base/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 53.33% <0.00%> (-46.67%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 95.45% <0.00%> (-4.55%) ⬇️
...entelemetry-sdk-trace-web/src/WebTracerProvider.ts
packages/opentelemetry-sdk-trace-web/src/utils.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...-sdk-trace-web/src/enums/PerformanceTimingNames.ts
...telemetry-sdk-trace-web/src/StackContextManager.ts
... and 9 more

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Sep 12, 2022
@github-actions
Copy link

github-actions bot commented Oct 3, 2022

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Oct 3, 2022
@msimone-human
Copy link

Any plan to include this in future releases ? The size of the library is a show stopper for us to adopt it.

@dyladan
Copy link
Member

dyladan commented Oct 12, 2022

Yes this is planned. I would have accepted this PR had the CLA been signed. If you'd like to work on it, you can comment on #2855. We're aware that the bundle is prohibitively large for many web users. The early days (and honestly the current days) of this SDK were quite focused on NodeJS as most of the maintainers have significantly more node experience. Also, OTel in general (all languages) tends to be more server-side focused than client-side. We recognize that is a shortcoming and focus is shifting but progress is slow.

If you're interested, there is an effort by @MSNev to make the JS SDK more browser-friendly, which may even result in a browser-specific JS SDK. There is also a RUM working group which is working on additions/changes to the spec to make it more client friendly.

@msimone-human
Copy link

@pkanal was faster on the mark to grab this :-)

@pkanal pkanal mentioned this pull request Oct 13, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sideEffects: false to package.json for packages to enable tree shaking
3 participants