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

ref(utils): Make States a const enum #4210

Merged
merged 5 commits into from
Dec 2, 2021
Merged

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Dec 1, 2021

States is an interally used enum, so we do not need the runtime support. The States enum lives in packages/utils/src/syncpromise.ts and is used by SyncPromise to manage state internally.

According to the TS docs: https://www.typescriptlang.org/docs/handbook/enums.html

Const enums can only use constant enum expressions and unlike regular enums they are completely removed during compilation. Const enum members are inlined at use sites. This is possible since const enums cannot have computed members.

This helps save on bundle size.

`States` is an interally used enum, so we do not need the runtime
support. This helps save on bundle size.
@AbhiPrasad AbhiPrasad added this to the Treeshaking / Bundle Size milestone Dec 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2021

size-limit report

Path Base Size (cabb53c) Current Size Change
@sentry/browser - CDN Bundle (gzipped) 22.48 KB 22.45 KB -0.15% 🔽
@sentry/browser - Webpack 23.37 KB 23.29 KB -0.34% 🔽
@sentry/react - Webpack 23.4 KB 23.32 KB -0.34% 🔽
@sentry/nextjs Client - Webpack 49.73 KB 49.7 KB -0.07% 🔽
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.93 KB 29.9 KB -0.11% 🔽

@lobsterkatie
Copy link
Member

lobsterkatie commented Dec 2, 2021

...so we do not need the runtime support. This helps save on bundle size.

Suggestion: ...so we don't need its underlying values to be meaningful at runtime. Making it a const enum allows TS to convert those values to inlined integers, thereby decreasing bundle size.

You also might consider mentioning, in the title and/or the description, that States is part of SyncPromise, just so people don't have to click through to the Files tab to figure that out.

@AbhiPrasad
Copy link
Member Author

Updated description

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) December 2, 2021 16:51
@AbhiPrasad AbhiPrasad merged commit 3370277 into master Dec 2, 2021
@AbhiPrasad AbhiPrasad deleted the abhi-const-enum-states branch December 2, 2021 17:04
AbhiPrasad added a commit that referenced this pull request Dec 2, 2021
`States` is an interally used enum, so we do not need the runtime support. The `States` enum lives in `packages/utils/src/syncpromise.ts` and is used by `SyncPromise` to manage state internally.

According to the TS docs: https://www.typescriptlang.org/docs/handbook/enums.html 

> Const enums can only use constant enum expressions and unlike regular enums they are completely removed during compilation. Const enum members are inlined at use sites. This is possible since const enums cannot have computed members. 

This helps save on bundle size.
onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
`States` is an interally used enum, so we do not need the runtime support. The `States` enum lives in `packages/utils/src/syncpromise.ts` and is used by `SyncPromise` to manage state internally.

According to the TS docs: https://www.typescriptlang.org/docs/handbook/enums.html 

> Const enums can only use constant enum expressions and unlike regular enums they are completely removed during compilation. Const enum members are inlined at use sites. This is possible since const enums cannot have computed members. 

This helps save on bundle size.
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.

3 participants