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-730] prefer object and type alias over enum in APIs #630

Merged
merged 14 commits into from
Dec 14, 2020

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Nov 23, 2020

Motivation

make APIs easier to use with TypeScript by avoiding to import our enums

before

import { datadogRum, Datacenter } from '@datadog/browser-rum'

datadogRum.init({
  ...
  datacenter: Datacenter.US
  ...
})

after

import { datadogRum } from '@datadog/browser-rum'

datadogRum.init({
  ...
  datacenter: 'us'
  ...
})

Changes

replace enum usage in APIs by object and type alias

before

export enum Datacenter {	
  US = 'us',	  
  EU = 'eu',	  
}

after

export const Datacenter = {
  US: 'us',	  
  EU: 'eu',	  
} as const

export type Datacenter = (typeof Datacenter)[keyof typeof Datacenter]

Testing

ci


I have gone over the contributing documentation.

@bcaudan bcaudan changed the title 💥 prefer constant union over enum in APIs 💥 [RUMF-730] prefer constant union over enum in APIs Nov 24, 2020
reasoning: make APIs easier to use with TypeScript by using directly constants instead of importing our enums
@bcaudan bcaudan marked this pull request as ready for review November 24, 2020 10:33
@bcaudan bcaudan requested a review from a team as a code owner November 24, 2020 10:33
@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #630 (ffee4af) into bcaudan/v2 (cdf9af0) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           bcaudan/v2     #630      +/-   ##
==============================================
- Coverage       87.33%   87.25%   -0.08%     
==============================================
  Files              54       54              
  Lines            2440     2425      -15     
  Branches          512      508       -4     
==============================================
- Hits             2131     2116      -15     
  Misses            309      309              
Impacted Files Coverage Δ
packages/logs/src/boot/logs.entry.ts 100.00% <ø> (ø)
packages/core/src/boot/init.ts 68.57% <100.00%> (-1.70%) ⬇️
packages/core/src/tools/error.ts 100.00% <100.00%> (ø)
packages/logs/src/domain/logger.ts 95.23% <100.00%> (-0.69%) ⬇️
packages/rum/src/boot/rum.entry.ts 92.06% <100.00%> (ø)
...omain/rumEventsCollection/error/errorCollection.ts 100.00% <100.00%> (ø)

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 cdf9af0...ffee4af. Read the comment docs.

@BenoitZugmeyer
Copy link
Member

IMO it would be simpler to completely remove enum (ErrorSource, StatusType, HandlerType...) and use the string instead. Or, we could also use plain constants like:

const SOURCE_AGENT = 'agent' as const

This would result in a smaller code output, and avoid having two "types" for the same values (ex: ErrorSource and Source, HandlerType and Handler...)

WDYT?

@bcaudan bcaudan changed the base branch from bcaudan/v2 to master December 14, 2020 10:59
@bcaudan bcaudan changed the title 💥 [RUMF-730] prefer constant union over enum in APIs 💥 [RUMF-730] prefer object and type alias over enum in APIs Dec 14, 2020
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

LGTM!

@bcaudan bcaudan merged commit 2a5a323 into master Dec 14, 2020
@bcaudan bcaudan deleted the bcaudan/union-constant branch December 14, 2020 15:49
@aleksandrlat
Copy link

Oh no why did you remove types? We need to duplicate type on our own now :(

@bcaudan
Copy link
Contributor Author

bcaudan commented Dec 16, 2020

discussion moved to #654

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