ref(types): Stop using `Severity` enum #4926

lobsterkatie · 2022-04-12T19:58:35Z

Note: This and #4896 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch.

Our original v7 plan called for deprecating and then removing the Severity enum which lives in @sentry/types, because enums transpile to a two-way lookup function with a fairly hefty footprint (see this SO answer from one of the maintainers of TS). We therefore added a new SeverityLevel type, which is the union of all of Severity's underlying values, and directed people to use that instead. Though we subsequently decided not to remove Severity (at least in v7), we agreed that we should stop using it internally. This implements that change.

Key Changes:

Severity and severityFromString have been redeprecated.
The original plan to have SeverityLevel live in @sentry/utils has been reverted, and it now lives only in @sentry/types.
The internal SeverityLevels array on which we were basing SeverityLevel has been removed. While we lose the elegance of the derived type, we gain the ability to truly only export types from @sentry/types.
Wherever we accept a Severity value, we now also accept a SeverityLevel.
All internal uses of Severity values have been replaced with the equivalent string constants.
A new severityLevelFromString function has been introduced, and is now used in place of SeverityFromString.
The tests for severityFromString have been cleaned up and replaced with equivalent tests for SeverityLevelFromString.

github-actions · 2022-04-12T20:13:11Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.1 KB (-0.18% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	64.17 KB (-0.69% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.76 KB (-0.52% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	57.72 KB (-0.44% 🔽)
@sentry/browser - Webpack (gzipped + minified)	23.28 KB (+0.2% 🔺)
@sentry/browser - Webpack (minified)	80.93 KB (-0.96% 🔽)
@sentry/react - Webpack (gzipped + minified)	23.32 KB (+0.18% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.78 KB (-0.57% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.96 KB (-0.46% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.31 KB (-0.72% 🔽)

Lms24

Overall these changes LGTM. Let's see what the others think about the Severity deprecation related questions and adjust accordingly.

packages/utils/src/severity.ts

Lms24 · 2022-04-13T07:24:29Z

packages/utils/src/severity.ts

-    return level;
-  }
-  return Severity.Log;
+  return (level === 'warn' ? 'warning' : validSeverityLevels.includes(level) ? level : 'log') as Severity;


More of a question than a remark: Do we have a convention regarding nested ternary operators? I personally don't mind them (in fact I really like using ternary operators) but I know that this is sort of a debated topic amongst people...

We do not. I think it's not that hard to get carried away with them (see below), so I try to use them fairly sparingly, but sometimes, especially for short stuff, I think they're fine.

Real code; not ours

packages/utils/src/severity.ts

lforst · 2022-04-13T08:52:21Z

packages/utils/src/severity.ts

-import { SeverityLevel, SeverityLevels } from './enums';
+// TODO: Should `severityFromString` be deprecated?
+
+// Note: Ideally the `SeverityLevel` type would be derived from `validSeverityLevels`, but that would mean either


I kind of disagree that SeverityLevel should be derived from an array. Is this just so severityFromString is updated when we update the severities? I believe that just introduces headaches with dependencies.
I'd like for us to try to just keep Severity types from becoming anything runtime related.

I think we could get get rid of the the problems with a) and c) if we

Delete validSeverityLevels here

Add a type (not const!) in @sentry/types which looks like export type ValidSeverityLevels = ['fatal', 'error', ...]

Change SeverityLevel in @sentry/types to export type SeverityLevel = ValidSeverityLevels[number];

Define a local const in this file that looks like const validSeverityLevels: ValidSeverityLevels = ['fatal', 'error', ...] (We don't export that const)

That way we will throw a type error here if there is a mismatch in implementation (ie. if we forget to update something when changing severity levels).

We'd have to update the severityFromString function a tiny bit to make linters stop screaming but I think this would be more elegant than having a type derived from an actual const.

Edit: I just now realize that this comment is a hypothetical and a reminder lol.

Edit: I just now realize that this comment is a hypothetical and a reminder lol.

Indeed. Also, everything you said, minus the last bullet point, is actually exactly how things are right now (prior to this PR). This note is partially inspired by that fact, like you said, as a reminder not to go back there.

I believe right now we export a const SeverityLevels from the types package. I meant that I think we should convert that to type SeverityLevels. That should probably also save some bytes now that I think of it 🤔

Wherever it lives, and whatever we call it, it has to exist as an actual concrete array, for use in SeverityLevelFromString. So we don't get to save those bytes, unfortunately.

lforst · 2022-04-13T09:01:42Z

packages/utils/src/severity.ts

+// create a circular dependency between `@sentry/types` and `@sentry/utils` (also not good). So a TODO accompanying the
+// type, reminding anyone who changes it to change this list also, will have to do.
+
+export const validSeverityLevels = ['fatal', 'error', 'warning', 'log', 'info', 'debug', 'critical'];


If we export this it leaks into the API. Do we want this just for a test?

Totally with you. This is a perfect example of why I want to go to selective exports in the utils package.

For the moment I'm going to leave this be, until we either do that or decide we're not doing that.

Note: This and #4926 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. This applies [the TypeScript `isolatedModules` setting[1] to all packages in the repo, which is necessary in order for us to use any compiler other than `tsc`. (All other compilers handle each file separately, without understanding the relationships between them the way `tsc` does, so we need TS to warn us if we're doing anything which would make that a problem). It also makes the second of two code changes necessary in order to be compatible with the `isolatedModules` flag: all re-exported types and interfaces (anything that will get stripped away when compiling from TS to JS) are now exported using `export type`. This lets non-`tsc` compilers know that the exported types are eligible for stripping, in spite of the fact that said compilers can't follow the export path backwards to see the types' implementation. (The other code change, eliminating our usage of the `Severity` enum, was split off into the PR linked above.) [1] https://www.typescriptlang.org/tsconfig#isolatedModules

Note: This and #4896 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. Our original v7 plan called for deprecating and then removing the `Severity` enum which lives in `@sentry/types`, because enums transpile to a two-way lookup function with a fairly hefty footprint (see this SO answer[1] from one of the maintainers of TS). We therefore added a new `SeverityLevel` type, which is the union of all of `Severity`'s underlying values, and directed people to use that instead. Though we subsequently decided not to remove `Severity` (at least in v7), we agreed that we should stop using it internally. This implements that change. Key Changes: - `Severity` and `severityFromString` have been redeprecated. - The original plan to have `SeverityLevel` live in `@sentry/utils` has been reverted, and it now lives only in `@sentry/types`. - The internal `SeverityLevels` array on which we were basing `SeverityLevel` has been removed. While we lose the elegance of the derived type, we gain the ability to truly only export types from `@sentry/types`. - Wherever we accept a `Severity` value, we now also accept a `SeverityLevel`. - All internal uses of `Severity` values have been replaced with the equivalent string constants. - A new `severityLevelFromString` function has been introduced, and is now used in place of `SeverityFromString`. - The tests for `severityFromString` have been cleaned up and replaced with equivalent tests for `SeverityLevelFromString`. [1] https://stackoverflow.com/a/28818850

Note: This and #4926 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. This applies [the TypeScript `isolatedModules` setting[1] to all packages in the repo, which is necessary in order for us to use any compiler other than `tsc`. (All other compilers handle each file separately, without understanding the relationships between them the way `tsc` does, so we need TS to warn us if we're doing anything which would make that a problem). It also makes the second of two code changes necessary in order to be compatible with the `isolatedModules` flag: all re-exported types and interfaces (anything that will get stripped away when compiling from TS to JS) are now exported using `export type`. This lets non-`tsc` compilers know that the exported types are eligible for stripping, in spite of the fact that said compilers can't follow the export path backwards to see the types' implementation. (The other code change, eliminating our usage of the `Severity` enum, was split off into the PR linked above.) [1] https://www.typescriptlang.org/tsconfig#isolatedModules

Note: This and #4896 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. Our original v7 plan called for deprecating and then removing the `Severity` enum which lives in `@sentry/types`, because enums transpile to a two-way lookup function with a fairly hefty footprint (see this SO answer[1] from one of the maintainers of TS). We therefore added a new `SeverityLevel` type, which is the union of all of `Severity`'s underlying values, and directed people to use that instead. Though we subsequently decided not to remove `Severity` (at least in v7), we agreed that we should stop using it internally. This implements that change. Key Changes: - `Severity` and `severityFromString` have been redeprecated. - The original plan to have `SeverityLevel` live in `@sentry/utils` has been reverted, and it now lives only in `@sentry/types`. - The internal `SeverityLevels` array on which we were basing `SeverityLevel` has been removed. While we lose the elegance of the derived type, we gain the ability to truly only export types from `@sentry/types`. - Wherever we accept a `Severity` value, we now also accept a `SeverityLevel`. - All internal uses of `Severity` values have been replaced with the equivalent string constants. - A new `severityLevelFromString` function has been introduced, and is now used in place of `SeverityFromString`. - The tests for `severityFromString` have been cleaned up and replaced with equivalent tests for `SeverityLevelFromString`. [1] https://stackoverflow.com/a/28818850

Note: This and #4926 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. This applies [the TypeScript `isolatedModules` setting[1] to all packages in the repo, which is necessary in order for us to use any compiler other than `tsc`. (All other compilers handle each file separately, without understanding the relationships between them the way `tsc` does, so we need TS to warn us if we're doing anything which would make that a problem). It also makes the second of two code changes necessary in order to be compatible with the `isolatedModules` flag: all re-exported types and interfaces (anything that will get stripped away when compiling from TS to JS) are now exported using `export type`. This lets non-`tsc` compilers know that the exported types are eligible for stripping, in spite of the fact that said compilers can't follow the export path backwards to see the types' implementation. (The other code change, eliminating our usage of the `Severity` enum, was split off into the PR linked above.) [1] https://www.typescriptlang.org/tsconfig#isolatedModules

Note: This and #4896 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. Our original v7 plan called for deprecating and then removing the `Severity` enum which lives in `@sentry/types`, because enums transpile to a two-way lookup function with a fairly hefty footprint (see this SO answer[1] from one of the maintainers of TS). We therefore added a new `SeverityLevel` type, which is the union of all of `Severity`'s underlying values, and directed people to use that instead. Though we subsequently decided not to remove `Severity` (at least in v7), we agreed that we should stop using it internally. This implements that change. Key Changes: - `Severity` and `severityFromString` have been redeprecated. - The original plan to have `SeverityLevel` live in `@sentry/utils` has been reverted, and it now lives only in `@sentry/types`. - The internal `SeverityLevels` array on which we were basing `SeverityLevel` has been removed. While we lose the elegance of the derived type, we gain the ability to truly only export types from `@sentry/types`. - Wherever we accept a `Severity` value, we now also accept a `SeverityLevel`. - All internal uses of `Severity` values have been replaced with the equivalent string constants. - A new `severityLevelFromString` function has been introduced, and is now used in place of `SeverityFromString`. - The tests for `severityFromString` have been cleaned up and replaced with equivalent tests for `SeverityLevelFromString`. [1] https://stackoverflow.com/a/28818850

Note: This and #4926 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. This applies [the TypeScript `isolatedModules` setting[1] to all packages in the repo, which is necessary in order for us to use any compiler other than `tsc`. (All other compilers handle each file separately, without understanding the relationships between them the way `tsc` does, so we need TS to warn us if we're doing anything which would make that a problem). It also makes the second of two code changes necessary in order to be compatible with the `isolatedModules` flag: all re-exported types and interfaces (anything that will get stripped away when compiling from TS to JS) are now exported using `export type`. This lets non-`tsc` compilers know that the exported types are eligible for stripping, in spite of the fact that said compilers can't follow the export path backwards to see the types' implementation. (The other code change, eliminating our usage of the `Severity` enum, was split off into the PR linked above.) [1] https://www.typescriptlang.org/tsconfig#isolatedModules

This switches our `build` and `build:dev` yarn scripts to use the new rollup/sucrase build process. This is the culmination of a number of previous changes, whose highlights include: - #4724, which made building types files a separate build process - #4895, which updated the SDK to use TS 3.8.3 - #4926, which removed use of the `Severity` enum - #5005, which switch our default tsconfig to target es6 - #4992, which added the Sucrase plugin, some helper functions, and the `yarn build:rollup` script - #4993, which added rollup plugins to use `var` rather than `const` and clean up the built code in various ways - #5022, which applied the same `const`-to-`var` translation to tests - #5023, which added the ability to change injected polyfills into imports The result is that, as of this PR, we will no longer use `tsc` to transpile or down-complile our code when building npm packages. Instead, we will be using Rollup to handle making our code CJS-friendlly and Sucrase to handle the transpilation from TS to JS. The main advantages of this change are: - It forced us to do a lot of updating, centralizing, and cleanup of our tooling, not just for build but also for testing and linting. - It brings our CDN build process and our npm build process much more in line with each other, for easier maintainability. - It gives us more control over the eventual output, because we have access to a whole internet full of Rollup plugins (not to mention the ability to make our own), rather than being constrained to tsconfig options. (Plugins also allow us to interact with the code directly.) - It speeds up our builds fairly significantly. I ran a number of trials in GHA of running `yarn build:dev` at the top level of the repo. Before this change, the average time was ~150 seconds. After this change, it's about half that, roughly 75 seconds. Because of the switch in tooling, the code we publish is going to be slightly different. In order to make sure that those differences weren't going to be breaking, I built each package under the old system and under the new system, ran a `git diff`, and checked every file, both CJS and ESM, in every package affected by this change. The differences (none of which affect behavior or eventual bundle size by more than a few bytes in each direction), fell into a few categories: - Purely cosmetic changes, things like which comments are retained, the order of imports, where in the file exports live, etc. - Changes to class constructors, things like not explicitly assigning `undefined` to undefined attributes, using regular assignment rather than `Object.defineProperty` for attributes which are assigned values, and splitting some of those assignments off into helper functions. - Changes related to the upgrade to ES6 and dropping of support for Node 6, things like not polyfilling object spread or async/await While this represents the most significant part of the overall change, a few outstanding tasks remain: - Making this same switch in `build:watch` - Parallelizing the builds, both locally and in CI - Perhaps applying the new process to our CDN bundle builds - Generalized cleanup These will all be included in separate PRs, some in the immediate future and some in the hopefully-not-too-distant short-to-medium term.

Note: This and #4896 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. Our original v7 plan called for deprecating and then removing the `Severity` enum which lives in `@sentry/types`, because enums transpile to a two-way lookup function with a fairly hefty footprint (see this SO answer[1] from one of the maintainers of TS). We therefore added a new `SeverityLevel` type, which is the union of all of `Severity`'s underlying values, and directed people to use that instead. Though we subsequently decided not to remove `Severity` (at least in v7), we agreed that we should stop using it internally. This implements that change. Key Changes: - `Severity` and `severityFromString` have been redeprecated. - The original plan to have `SeverityLevel` live in `@sentry/utils` has been reverted, and it now lives only in `@sentry/types`. - The internal `SeverityLevels` array on which we were basing `SeverityLevel` has been removed. While we lose the elegance of the derived type, we gain the ability to truly only export types from `@sentry/types`. - Wherever we accept a `Severity` value, we now also accept a `SeverityLevel`. - All internal uses of `Severity` values have been replaced with the equivalent string constants. - A new `severityLevelFromString` function has been introduced, and is now used in place of `SeverityFromString`. - The tests for `severityFromString` have been cleaned up and replaced with equivalent tests for `SeverityLevelFromString`. [1] https://stackoverflow.com/a/28818850

Note: This and #4926 are together a (slightly updated) repeat of #4497, to get it onto the main v7 branch. This applies [the TypeScript `isolatedModules` setting[1] to all packages in the repo, which is necessary in order for us to use any compiler other than `tsc`. (All other compilers handle each file separately, without understanding the relationships between them the way `tsc` does, so we need TS to warn us if we're doing anything which would make that a problem). It also makes the second of two code changes necessary in order to be compatible with the `isolatedModules` flag: all re-exported types and interfaces (anything that will get stripped away when compiling from TS to JS) are now exported using `export type`. This lets non-`tsc` compilers know that the exported types are eligible for stripping, in spite of the fact that said compilers can't follow the export path backwards to see the types' implementation. (The other code change, eliminating our usage of the `Severity` enum, was split off into the PR linked above.) [1] https://www.typescriptlang.org/tsconfig#isolatedModules

This switches our `build` and `build:dev` yarn scripts to use the new rollup/sucrase build process. This is the culmination of a number of previous changes, whose highlights include: - #4724, which made building types files a separate build process - #4895, which updated the SDK to use TS 3.8.3 - #4926, which removed use of the `Severity` enum - #5005, which switch our default tsconfig to target es6 - #4992, which added the Sucrase plugin, some helper functions, and the `yarn build:rollup` script - #4993, which added rollup plugins to use `var` rather than `const` and clean up the built code in various ways - #5022, which applied the same `const`-to-`var` translation to tests - #5023, which added the ability to change injected polyfills into imports The result is that, as of this PR, we will no longer use `tsc` to transpile or down-complile our code when building npm packages. Instead, we will be using Rollup to handle making our code CJS-friendlly and Sucrase to handle the transpilation from TS to JS. The main advantages of this change are: - It forced us to do a lot of updating, centralizing, and cleanup of our tooling, not just for build but also for testing and linting. - It brings our CDN build process and our npm build process much more in line with each other, for easier maintainability. - It gives us more control over the eventual output, because we have access to a whole internet full of Rollup plugins (not to mention the ability to make our own), rather than being constrained to tsconfig options. (Plugins also allow us to interact with the code directly.) - It speeds up our builds fairly significantly. I ran a number of trials in GHA of running `yarn build:dev` at the top level of the repo. Before this change, the average time was ~150 seconds. After this change, it's about half that, roughly 75 seconds. Because of the switch in tooling, the code we publish is going to be slightly different. In order to make sure that those differences weren't going to be breaking, I built each package under the old system and under the new system, ran a `git diff`, and checked every file, both CJS and ESM, in every package affected by this change. The differences (none of which affect behavior or eventual bundle size by more than a few bytes in each direction), fell into a few categories: - Purely cosmetic changes, things like which comments are retained, the order of imports, where in the file exports live, etc. - Changes to class constructors, things like not explicitly assigning `undefined` to undefined attributes, using regular assignment rather than `Object.defineProperty` for attributes which are assigned values, and splitting some of those assignments off into helper functions. - Changes related to the upgrade to ES6 and dropping of support for Node 6, things like not polyfilling object spread or async/await While this represents the most significant part of the overall change, a few outstanding tasks remain: - Making this same switch in `build:watch` - Parallelizing the builds, both locally and in CI - Perhaps applying the new process to our CDN bundle builds - Generalized cleanup These will all be included in separate PRs, some in the immediate future and some in the hopefully-not-too-distant short-to-medium term.

lobsterkatie force-pushed the kmclb-stop-using-severity-enum branch from 94e403e to 7e4e2de Compare April 13, 2022 05:06

lobsterkatie mentioned this pull request Apr 13, 2022

ref(build): Turn on isolatedModules TS option #4896

Merged

Lms24 reviewed Apr 13, 2022

View reviewed changes

lforst reviewed Apr 13, 2022

View reviewed changes

lobsterkatie force-pushed the kmclb-stop-using-severity-enum branch 2 times, most recently from 4b0d4d6 to a042bed Compare April 13, 2022 16:42

AbhiPrasad approved these changes Apr 13, 2022

View reviewed changes

lobsterkatie force-pushed the kmclb-stop-using-severity-enum branch 3 times, most recently from bf0dc01 to dc09452 Compare April 13, 2022 23:58

lobsterkatie added 12 commits April 13, 2022 17:00

remove SeverityLevels

c9e7aa6

make SeverityLevel live only in @sentry/types

559ec01

simplify severityFromString

fba7c2a

add comments and fix docstring

fdcff7d

add severityLevelFromString function

c5c7935

remove duplicate tests

0c2e50b

simplify test structure and fix test names

a10f8ad

accept SeverityLevel wherever we accept Severity

31a4c3d

replace uses of Severity enum with equivalent strings

9f394a0

replace uses of severityFromString with severityLevelFromString

712a631

redeprecate Severity and severityFromString

6cd3203

ignore Severity deprecation linter errors

caf44a3

lobsterkatie force-pushed the kmclb-stop-using-severity-enum branch from dc09452 to caf44a3 Compare April 14, 2022 00:00

lobsterkatie merged commit 292075f into 7.x Apr 14, 2022

lobsterkatie deleted the kmclb-stop-using-severity-enum branch April 14, 2022 00:32

lforst mentioned this pull request Apr 14, 2022

ref(javascript): Replace deprecated severity enums with string literals getsentry/sentry-docs#4940

Merged

lobsterkatie added this to the 7.0.0 milestone May 9, 2022

lobsterkatie mentioned this pull request May 9, 2022

ref(build): Use rollup/sucrase for non-watch npm builds #5035

Merged

JavierMartinz mentioned this pull request May 30, 2022

Stop using Severity enum DiederikvandenB/apollo-link-sentry#431

Merged

liamjones mentioned this pull request Jun 29, 2022

feat: add support for API v7 zivl/sentry-testkit#109

Merged

AbhiPrasad mentioned this pull request Feb 7, 2024

feat(v8): Remove Severity enum #10551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(types): Stop using `Severity` enum #4926

ref(types): Stop using `Severity` enum #4926

lobsterkatie commented Apr 12, 2022 •

edited

Loading

github-actions bot commented Apr 12, 2022 •

edited

Loading

Lms24 left a comment

Lms24 Apr 13, 2022

lobsterkatie Apr 13, 2022

lforst Apr 13, 2022 •

edited

Loading

lobsterkatie Apr 13, 2022

lforst Apr 13, 2022 •

edited

Loading

lobsterkatie Apr 14, 2022

lforst Apr 13, 2022

lobsterkatie Apr 13, 2022 •

edited

Loading

ref(types): Stop using Severity enum #4926

ref(types): Stop using Severity enum #4926

Conversation

lobsterkatie commented Apr 12, 2022 • edited Loading

github-actions bot commented Apr 12, 2022 • edited Loading

size-limit report 📦

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Apr 13, 2022

Choose a reason for hiding this comment

lobsterkatie Apr 13, 2022

Choose a reason for hiding this comment

lforst Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

lobsterkatie Apr 13, 2022

Choose a reason for hiding this comment

lforst Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

lobsterkatie Apr 14, 2022

Choose a reason for hiding this comment

lforst Apr 13, 2022

Choose a reason for hiding this comment

lobsterkatie Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

ref(types): Stop using `Severity` enum #4926

ref(types): Stop using `Severity` enum #4926

lobsterkatie commented Apr 12, 2022 •

edited

Loading

github-actions bot commented Apr 12, 2022 •

edited

Loading

lforst Apr 13, 2022 •

edited

Loading

lforst Apr 13, 2022 •

edited

Loading

lobsterkatie Apr 13, 2022 •

edited

Loading