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

💥 remove Object.* Polyfills #2908

Merged
merged 22 commits into from
Aug 1, 2024
Merged

💥 remove Object.* Polyfills #2908

merged 22 commits into from
Aug 1, 2024

Conversation

thomas-lebeau
Copy link
Collaborator

@thomas-lebeau thomas-lebeau commented Jul 31, 2024

Motivation

Removed unnecessary polyfills for v6, resulting in a slight improvement in the bundle sizes.

Changes

  • Remove objectValues and objectEntries polyfills. However I'm keeping the wrapper method because their mangling result in smaller bundle size than using Object.values() every time it is used.
  • Remove Object.assign polyfills in favor of object spread operator.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@thomas-lebeau thomas-lebeau changed the base branch from main to v6 July 31, 2024 12:17
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2024

Codecov Report

Attention: Patch coverage is 91.13924% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.76%. Comparing base (c94a365) to head (a0617ef).

Files Patch % Lines
packages/core/src/boot/init.ts 33.33% 2 Missing ⚠️
packages/logs/src/boot/logsPublicApi.ts 33.33% 2 Missing ⚠️
packages/rum-core/src/boot/rumPublicApi.ts 33.33% 2 Missing ⚠️
.../resource/retrieveInitialDocumentResourceTiming.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v6    #2908      +/-   ##
==========================================
- Coverage   93.76%   93.76%   -0.01%     
==========================================
  Files         267      267              
  Lines        7378     7372       -6     
  Branches     1659     1658       -1     
==========================================
- Hits         6918     6912       -6     
  Misses        460      460              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomas-lebeau thomas-lebeau changed the base branch from v6 to thomas.lebeau/remove-pollyfills August 1, 2024 06:35
Copy link

cit-pr-commenter bot commented Aug 1, 2024

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 149.42 KiB 149.35 KiB -71 B -0.05%
Logs 50.78 KiB 50.66 KiB -122 B -0.23%
Rum Slim 99.38 KiB 99.31 KiB -67 B -0.07%
Worker 25.16 KiB 25.16 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.002 0.002 0.001
addaction 0.031 0.037 0.006
adderror 0.030 0.034 0.004
addtiming 0.001 0.001 0.000
startview 0.847 1.003 0.156
startstopsessionreplayrecording 0.824 0.727 -0.097
logmessage 0.020 0.018 -0.002
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 22.30 KiB 21.85 KiB -461 B
addaction 71.21 KiB 71.64 KiB 435 B
adderror 85.84 KiB 85.09 KiB -769 B
addtiming 19.49 KiB 19.08 KiB -415 B
startview 336.36 KiB 337.16 KiB 826 B
startstopsessionreplayrecording 16.13 KiB 16.00 KiB -133 B
logmessage 71.88 KiB 71.57 KiB -322 B

🔗 RealWorld

@thomas-lebeau thomas-lebeau force-pushed the thomas.lebeau/polyfill-s branch from 636aba9 to 72edfea Compare August 1, 2024 08:06
@thomas-lebeau thomas-lebeau mentioned this pull request Aug 1, 2024
4 tasks
@@ -161,7 +160,7 @@ type GenericBeforeSendCallback = (event: any, context?: any) => unknown
*/
type ProxyFn = (options: { path: string; parameters: string }) => string

interface ReplicaUserConfiguration {
export interface ReplicaUserConfiguration {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks weird but it's because this some type is inferred as ReplicaUserConfiguration in another module (remoteConfiguration.ts) but typescript can't name it.

@datadog/browser-rum-core: src/domain/configuration/remoteConfiguration.ts(22,17): error TS4058: Return type of exported function has or is using name 'ReplicaUserConfiguration' from external module "/Users/thomas.lebeau/go/src/github.com/DataDog/browser-sdk/packages/core/cjs/domain/configuration/configuration" but cannot be named.

Copy link
Contributor

Choose a reason for hiding this comment

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

assign({}, initConfiguration, remoteInitConfiguration) return type is the intersection of RumInitConfiguration and RumRemoteConfiguration. So ReplicaUserConfiguration is not exposed.

And I think that { ...initConfiguration, ...remoteInitConfiguration } export a new type containing all the props from both object types, including non exported ReplicaUserConfiguration.

Maybe that's the reason.

Base automatically changed from thomas.lebeau/remove-pollyfills to v6 August 1, 2024 10:56
export function shallowClone<T>(object: T): T & Record<string, never> {
return assign({}, object)
return { ...object } as T & Record<string, never>
Copy link
Collaborator Author

@thomas-lebeau thomas-lebeau Aug 1, 2024

Choose a reason for hiding this comment

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

Somehow, if T is undefined, then TS will infer {...object} as undefined, insead of {}

see TS playground

@@ -315,23 +314,21 @@ function newClick(
}

const { resourceCount, errorCount, longTaskCount } = eventCountsSubscription.eventCounts
const clickAction: ClickAction = assign(
{
type: ActionType.CLICK as const,
Copy link
Collaborator Author

@thomas-lebeau thomas-lebeau Aug 1, 2024

Choose a reason for hiding this comment

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

type is unnecessary here, actually TS errors with: 'type' is specified more than once, so this usage will be overwritten. ts(2783)

spreading clickActionBase will always overwrites type

@thomas-lebeau thomas-lebeau marked this pull request as ready for review August 1, 2024 11:36
@thomas-lebeau thomas-lebeau requested a review from a team as a code owner August 1, 2024 11:36
@thomas-lebeau thomas-lebeau changed the title 💥 remove objectValues and objectEntries Polyfills 💥 remove Object.* Polyfills Aug 1, 2024
@@ -161,7 +160,7 @@ type GenericBeforeSendCallback = (event: any, context?: any) => unknown
*/
type ProxyFn = (options: { path: string; parameters: string }) => string

interface ReplicaUserConfiguration {
export interface ReplicaUserConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

assign({}, initConfiguration, remoteInitConfiguration) return type is the intersection of RumInitConfiguration and RumRemoteConfiguration. So ReplicaUserConfiguration is not exposed.

And I think that { ...initConfiguration, ...remoteInitConfiguration } export a new type containing all the props from both object types, including non exported ReplicaUserConfiguration.

Maybe that's the reason.

@thomas-lebeau thomas-lebeau merged commit c745086 into v6 Aug 1, 2024
17 checks passed
@thomas-lebeau thomas-lebeau deleted the thomas.lebeau/polyfill-s branch August 1, 2024 14:21
thomas-lebeau added a commit that referenced this pull request Dec 13, 2024
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