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

⚡️ [RUMF-1043] remove TSLib dependency #1347

Merged
merged 8 commits into from
Feb 28, 2022
Merged

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Feb 17, 2022

Motivation

We observed that sometimes, our Browser SDK NPM packages is used with a bundler configuration which does not always remove unused parts of TSLib (ex: when using Webpack with a resolve.mainFields set to "main"). In this case, TSLib adds a large amount of unused code to the user bundle. This is one of the actions identified to reduce the SDK size.

Our TSLib version is also a bit old, and our users are probably using a newer, incompatible version, which could produce duplicate code in the bundle (see #1183).

Changes

This PR removes the TSLib dependency and replace its usage with inlined TS helpers via the importHelpers TS config.

By doing so, the bundle grows a bit because helpers are duplicated in various modules. To mitigate this, this PR also forbid two syntaxes (object and array spread) that are known to produce such helpers (respectively assign and spreadArrays).

Note: some TS helpers are still used:

  • decorate, used by the Logger class. Because it is used a single time, no need to change / factorize it.

  • commonjs module declaration helpers, which are used when using tsc to produce commonjs modules. Those can't really be replaced with alternative code without important drawbacks. In total, those two functions are duplicated 4 times in core and 3 times in rum. This may be improved by shiping pre-built NPM packages.

Testing

To verify that our build does not produce more code than before, I run yarn build on both the PR branch and on main (in two different directories), and compare the result with something like:

diff -x '*.map' -u -r ../browser-sdk2/packages/rum/esm packages/rum/esm

The difference should be minimal.

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

This commit makes tsc output its helpers directly in each module files,
instead of trying to import tslib.
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners February 17, 2022 17:54
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

Merging #1347 (437cf84) into main (bf16a68) will increase coverage by 0.07%.
The diff coverage is 86.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1347      +/-   ##
==========================================
+ Coverage   91.07%   91.14%   +0.07%     
==========================================
  Files         104      104              
  Lines        4268     4268              
  Branches      950      950              
==========================================
+ Hits         3887     3890       +3     
+ Misses        381      378       -3     
Impacted Files Coverage Δ
packages/rum-core/src/domain/internalContext.ts 90.90% <ø> (ø)
...rumEventsCollection/resource/resourceCollection.ts 97.22% <ø> (ø)
packages/rum/src/constants.ts 100.00% <ø> (ø)
packages/core/src/boot/init.ts 84.61% <33.33%> (ø)
...ages/rum-core/src/browser/performanceCollection.ts 14.14% <33.33%> (ø)
packages/rum/src/domain/record/record.ts 75.00% <60.00%> (+5.55%) ⬆️
packages/core/src/browser/xhrObservable.ts 98.03% <100.00%> (ø)
...ges/core/src/domain/configuration/configuration.ts 100.00% <100.00%> (ø)
...src/domain/configuration/transportConfiguration.ts 100.00% <100.00%> (ø)
...kages/core/src/domain/console/consoleObservable.ts 100.00% <100.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf16a68...437cf84. Read the comment docs.

@amortemousque
Copy link
Collaborator

💭 thought: ‏I get the gain of this PR but of course I'm not a big fan to forbid object and array spread.
Do we agree that if we expose pre build packages for NPM users or remove the support of IE11 we will be able to use them again?

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.

4 participants