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

πŸ‘· Removing lodash dependencies #396

Merged
merged 6 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions LICENSE-3rdparty.csv
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
Component,Origin,License,Copyright
require,lodash.assign,MIT,Copyright jQuery Foundation and other contributors
require,lodash.merge,MIT,Copyright OpenJS Foundation and other contributors
require,tslib,Apache-2.0,Copyright Microsoft Corporation
file,tracekit,MIT,Copyright 2013 Onur Can Cakmak and all TraceKit contributors
dev,@types/jasmine,MIT,Copyright Microsoft Corporation
dev,@types/lodash.assign,MIT,Copyright Microsoft Corporation
dev,@types/lodash.merge,MIT,Copyright Microsoft Corporation
dev,@types/request,MIT,Copyright Microsoft Corporation
dev,@types/sinon,MIT,Copyright Microsoft Corporation
dev,@wdio/browserstack-service,MIT,Copyright JS Foundation and other contributors
Expand Down
4 changes: 0 additions & 4 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,9 @@
"build:esm": "rm -rf esm && tsc -p tsconfig.esm.json"
},
"dependencies": {
"lodash.assign": "4.2.0",
"lodash.merge": "4.6.2",
"tslib": "1.10.0"
},
"devDependencies": {
"@types/lodash.assign": "4.2.6",
"@types/lodash.merge": "4.6.6",
"@types/sinon": "7.0.13",
"sinon": "7.3.2"
},
Expand Down
10 changes: 3 additions & 7 deletions packages/core/src/internalMonitoring.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
// tslint:disable ban-types

import lodashAssign from 'lodash.assign'
import lodashMerge from 'lodash.merge'

import { Configuration } from './configuration'
import { toStackTraceString } from './errorCollection'
import { computeStackTrace } from './tracekit'
Expand Down Expand Up @@ -45,7 +41,7 @@ export function startInternalMonitoring(configuration: Configuration): InternalM
configuration.maxMessageSize,
configuration.flushTimeout,
() =>
lodashMerge(
utils.deepMerge(
{
date: new Date().getTime(),
view: {
Expand All @@ -54,10 +50,10 @@ export function startInternalMonitoring(configuration: Configuration): InternalM
},
},
externalContextProvider !== undefined ? externalContextProvider() : {}
)
) as utils.Context
)

lodashAssign(monitoringConfiguration, {
utils.assign(monitoringConfiguration, {
batch,
maxMessagesPerPage: configuration.maxInternalMonitoringMessagesPerPage,
sentMessageCount: 0,
Expand Down
6 changes: 2 additions & 4 deletions packages/core/src/transport.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import lodashMerge from 'lodash.merge'

import { monitor } from './internalMonitoring'
import { Context, DOM_EVENT, jsonStringify, noop, objectValues } from './utils'
import { Context, deepMerge, DOM_EVENT, jsonStringify, noop, objectValues } from './utils'

/**
* Use POST request without content type to:
Expand Down Expand Up @@ -89,7 +87,7 @@ export class Batch<T> {
}

private process(message: T) {
const contextualizedMessage = lodashMerge({}, this.contextProvider(), message) as Context
const contextualizedMessage = deepMerge({}, this.contextProvider(), (message as unknown) as Context) as Context
const processedMessage = jsonStringify(contextualizedMessage)!
const messageBytesSize = this.sizeInBytes(processedMessage)
return { processedMessage, messageBytesSize }
Expand Down
46 changes: 46 additions & 0 deletions packages/core/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,52 @@ export function throttle(
}
}

const isContextArray = (value: ContextValue): value is ContextArray => Array.isArray(value)
const isContext = (value: ContextValue): value is Context => !Array.isArray(value) && typeof value === 'object'

/**
* Performs a deep merge of objects and arrays
* - arrays values are merged index by index
* - objects are merged by keys
* - values get replaced, unless undefined
*
* ⚠️ this method does not prevent infinite loops while merging circular references ⚠️
*
*/
export function deepMerge(destination: ContextValue, ...toMerge: ContextValue[]): ContextValue {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
return toMerge.reduce((value1: ContextValue, value2: ContextValue): ContextValue => {
if (isContextArray(value1) && isContextArray(value2)) {
return [...Array(Math.max(value1.length, value2.length))].map((_, index) =>
deepMerge(value1[index], value2[index])
)
}
if (isContext(value1) && isContext(value2)) {
return Object.keys(value2).reduce(
Copy link
Member

Choose a reason for hiding this comment

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

A for/in loop or a (polyfilled) usage of Object.fromEntries would be easier to understand and faster here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really clear what's polyfilled and what is not πŸ˜… And I'm not sure ObjectEntries would be faster as it'll be doing an extra map on the array

Copy link
Member

Choose a reason for hiding this comment

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

It's not polyfilled. It would have a lower complexity, since you recreate the whole object for each key. Anyway, a for/in could do the trick :)

(merged, key) => ({
...merged,
[key]: deepMerge(value1[key], value2[key]),
}),
value1
)
}
return value2 === undefined ? value1 : value2
}, destination)
}

interface Assignable {
[key: string]: any
}

export function assign(target: Assignable, ...toAssign: Assignable[]) {
toAssign.forEach((source: Assignable) => {
for (const key in source) {
if (Object.prototype.hasOwnProperty.call(source, key)) {
target[key] = source[key]
}
}
})
}

/**
* UUID v4
* from https://gist.github.com/jed/982883
Expand Down
20 changes: 19 additions & 1 deletion packages/core/test/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
import { jsonStringify, performDraw, round, throttle, toSnakeCase, withSnakeCaseKeys } from '../src/utils'
import { deepMerge, jsonStringify, performDraw, round, throttle, toSnakeCase, withSnakeCaseKeys } from '../src/utils'

describe('utils', () => {
describe('deepMerge', () => {
it('should deeply add and replace keys', () => {
const target = { a: { b: 'toBeReplaced', c: 'target' } }
const source = { a: { b: 'replaced', d: 'source' } }
expect(deepMerge(target, source)).toEqual({ a: { b: 'replaced', c: 'target', d: 'source' } })
})

it('should not replace with undefined', () => {
expect(deepMerge({ a: 1 }, { a: undefined })).toEqual({ a: 1 })
})

it('should merge arrays', () => {
const target = [{ a: 'target' }, 'extraString']
const source = [{ b: 'source' }]
expect(deepMerge(target, source)).toEqual([{ a: 'target', b: 'source' }, 'extraString'])
})
})

describe('throttle', () => {
let spy: jasmine.Spy
let throttled: () => void
Expand Down
4 changes: 0 additions & 4 deletions packages/logs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,9 @@
},
"dependencies": {
"@datadog/browser-core": "1.11.5",
"lodash.assign": "4.2.0",
"lodash.merge": "4.6.2",
"tslib": "1.10.0"
},
"devDependencies": {
"@types/lodash.assign": "4.2.6",
"@types/lodash.merge": "4.6.6",
"@types/sinon": "7.0.13",
"sinon": "7.3.2"
},
Expand Down
14 changes: 7 additions & 7 deletions packages/logs/src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
Configuration,
Context,
ContextValue,
deepMerge,
ErrorMessage,
ErrorObservable,
ErrorOrigin,
Expand All @@ -12,7 +13,6 @@ import {
monitored,
noop,
} from '@datadog/browser-core'
import lodashMerge from 'lodash.merge'

import { LoggerSession } from './loggerSession'
import { LogsGlobal } from './logs.entry'
Expand Down Expand Up @@ -61,8 +61,8 @@ export function startLogger(
) {
let globalContext: Context = {}

internalMonitoring.setExternalContextProvider(() =>
lodashMerge({ session_id: session.getId() }, globalContext, getRUMInternalContext())
internalMonitoring.setExternalContextProvider(
() => deepMerge({ session_id: session.getId() }, globalContext, getRUMInternalContext() as Context) as Context
)

const batch = new Batch<LogsMessage>(
Expand All @@ -72,7 +72,7 @@ export function startLogger(
configuration.maxMessageSize,
configuration.flushTimeout,
() =>
lodashMerge(
deepMerge(
{
date: new Date().getTime(),
session_id: session.getId(),
Expand All @@ -82,7 +82,7 @@ export function startLogger(
},
},
globalContext,
getRUMInternalContext()
getRUMInternalContext() as Context
) as Context
)
const handlers = {
Expand Down Expand Up @@ -141,7 +141,7 @@ export class Logger {
@monitored
log(message: string, messageContext = {}, status = StatusType.info) {
if (this.session.isTracked() && STATUS_PRIORITIES[status] >= STATUS_PRIORITIES[this.level]) {
this.handler({ message, status, ...lodashMerge({}, this.loggerContext, messageContext) })
this.handler({ message, status, ...(deepMerge({}, this.loggerContext, messageContext) as Context) })
}
}

Expand All @@ -163,7 +163,7 @@ export class Logger {
origin: ErrorOrigin.LOGGER,
},
}
this.log(message, lodashMerge({}, errorOrigin, messageContext), StatusType.error)
this.log(message, deepMerge({}, errorOrigin, messageContext), StatusType.error)
}

setContext(context: Context) {
Expand Down
4 changes: 2 additions & 2 deletions packages/logs/src/logs.entry.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
areCookiesAuthorized,
assign,
checkIsNotLocalFile,
commonInit,
Context,
Expand All @@ -11,7 +12,6 @@ import {
monitor,
UserConfiguration,
} from '@datadog/browser-core'
import lodashAssign from 'lodash.assign'
import { buildEnv } from './buildEnv'
import { HandlerType, Logger, LoggerConfiguration, startLogger, StatusType } from './logger'
import { startLoggerSession } from './loggerSession'
Expand Down Expand Up @@ -94,7 +94,7 @@ datadogLogs.init = monitor((userConfiguration: LogsUserConfiguration) => {
const { errorObservable, configuration, internalMonitoring } = commonInit(logsUserConfiguration, buildEnv)
const session = startLoggerSession(configuration, areCookiesAuthorized())
const globalApi = startLogger(errorObservable, configuration, session, internalMonitoring)
lodashAssign(datadogLogs, globalApi)
assign(datadogLogs, globalApi)
isAlreadyInitialized = true
})

Expand Down
4 changes: 0 additions & 4 deletions packages/rum/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,9 @@
},
"dependencies": {
"@datadog/browser-core": "1.11.5",
"lodash.assign": "4.2.0",
"lodash.merge": "4.6.2",
"tslib": "1.10.0"
},
"devDependencies": {
"@types/lodash.assign": "4.2.6",
"@types/lodash.merge": "4.6.6",
"@types/sinon": "7.0.13",
"sinon": "7.3.2"
},
Expand Down
4 changes: 2 additions & 2 deletions packages/rum/src/rum.entry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
assign,
checkCookiesAuthorized,
checkIsNotLocalFile,
commonInit,
Expand All @@ -12,7 +13,6 @@ import {
startRequestCollection,
UserConfiguration,
} from '@datadog/browser-core'
import lodashAssign from 'lodash.assign'

import { buildEnv } from './buildEnv'
import { startDOMMutationCollection } from './domMutationCollection'
Expand Down Expand Up @@ -85,7 +85,7 @@ datadogRum.init = monitor((userConfiguration: RumUserConfiguration) => {
requestStartObservable.subscribe((startEvent) => lifeCycle.notify(LifeCycleEventType.REQUEST_STARTED, startEvent))
requestCompleteObservable.subscribe((request) => lifeCycle.notify(LifeCycleEventType.REQUEST_COMPLETED, request))

lodashAssign(datadogRum, globalApi)
assign(datadogRum, globalApi)
isAlreadyInitialized = true
})

Expand Down
25 changes: 13 additions & 12 deletions packages/rum/src/rum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
Configuration,
Context,
ContextValue,
deepMerge,
ErrorContext,
ErrorMessage,
getTimestamp,
Expand All @@ -18,7 +19,6 @@ import {
ResourceKind,
withSnakeCaseKeys,
} from '@datadog/browser-core'
import lodashMerge from 'lodash.merge'

import { LifeCycle, LifeCycleEventType } from './lifeCycle'
import { matchRequestTiming } from './matchRequestTiming'
Expand Down Expand Up @@ -153,17 +153,18 @@ export function startRum(
): Omit<RumGlobal, 'init'> {
let globalContext: Context = {}

internalMonitoring.setExternalContextProvider(() =>
lodashMerge(
{
application_id: applicationId,
session_id: viewContext.sessionId,
view: {
id: viewContext.id,
internalMonitoring.setExternalContextProvider(
() =>
deepMerge(
{
application_id: applicationId,
session_id: viewContext.sessionId,
view: {
id: viewContext.id,
},
},
},
globalContext
)
globalContext
) as Context
)

const batch = startRumBatch(
Expand Down Expand Up @@ -233,7 +234,7 @@ function startRumBatch(
configuration.batchBytesLimit,
configuration.maxMessageSize,
configuration.flushTimeout,
() => lodashMerge(withSnakeCaseKeys(rumContextProvider()), globalContextProvider()),
() => deepMerge(withSnakeCaseKeys(rumContextProvider()), globalContextProvider()) as Context,
beforeUnloadCallback
)
return {
Expand Down
28 changes: 6 additions & 22 deletions test/app/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,21 @@
# yarn lockfile v1


"@datadog/browser-core@1.8.3", "@datadog/browser-core@file:../../packages/core":
version "1.8.3"
"@datadog/browser-core@1.11.4", "@datadog/browser-core@file:../../packages/core":
version "1.11.4"
dependencies:
lodash.assign "4.2.0"
lodash.merge "4.6.2"
tslib "1.10.0"

"@datadog/browser-logs@file:../../packages/logs":
version "1.8.3"
version "1.11.4"
dependencies:
"@datadog/browser-core" "1.8.3"
lodash.assign "4.2.0"
lodash.merge "4.6.2"
"@datadog/browser-core" "1.11.4"
tslib "1.10.0"

"@datadog/browser-rum@file:../../packages/rum":
version "1.8.3"
version "1.11.4"
dependencies:
"@datadog/browser-core" "1.8.3"
lodash.assign "4.2.0"
lodash.merge "4.6.2"
"@datadog/browser-core" "1.11.4"
tslib "1.10.0"

"@webassemblyjs/ast@1.8.5":
Expand Down Expand Up @@ -1280,16 +1274,6 @@ locate-path@^3.0.0:
p-locate "^3.0.0"
path-exists "^3.0.0"

lodash.assign@4.2.0:
version "4.2.0"
resolved "https://registry.yarnpkg.com/lodash.assign/-/lodash.assign-4.2.0.tgz#0d99f3ccd7a6d261d19bdaeb9245005d285808e7"
integrity sha1-DZnzzNem0mHRm9rrkkUAXShYCOc=

lodash.merge@4.6.2:
version "4.6.2"
resolved "https://registry.yarnpkg.com/lodash.merge/-/lodash.merge-4.6.2.tgz#558aa53b43b661e1925a0afdfa36a9a1085fe57a"
integrity sha512-0KpjqXRVvrYyCsX1swR/XTK0va6VQkQM6MNo7PqW77ByjAhoARA8EfrP1N4+KlKj8YS0ZUCtRT/YUuhyYDujIQ==

lru-cache@^5.1.1:
version "5.1.1"
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-5.1.1.tgz#1da27e6710271947695daf6848e847f01d84b920"
Expand Down
Loading