ref(build): Update to TypeScript 3.8.3 #4895
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
AbhiPrasad
approved these changes
Apr 7, 2022
lobsterkatie
force-pushed
the
kmclb-upgrade-to-typescript-3.8.3
branch
from
361b9d8 to
08bdde6
Compare
Lms24
pushed a commit
that referenced
this pull request
Apr 26, 2022
This updates the SDK to use Typescript 3.8.3, in order to be able to use type-only imports and exports[1]. (These are needed for us to turn on `isolatedModules`, which is in turn is needed for us to switch our build tool away from `tsc`[2], since no other tool understands the relationship between files.)
As a result of this change, a few of the browser integration tests needed to be fixed so that all promises were explicitly awaited, a point about which the linter in 3.8 complains.
This is a breaking change for anyone using TS 3.7.x (there's no one using TS < 3.7.x, since that's our current minimum). That said, though there are plenty of public projects on GH using TS 3.7 and `@sentry/xyz`, if you restrict it to projects using TS 3.7 and `@sentry/xyz` 6.x, all you get are forks of this very repo. Granted, this isn't every project ever, but it's likely decently representative of the fact that if you've upgraded our SDK, you've almost certainly upgraded TS as well. We're going to wait until v7 to release this change in any case, but that's an indication that it won't affect many people when we do. (See this commit's PR for links to searches demonstrating the points.)
Note: Originally there was some thought of going farther, into TS 4.x, but we decided that for the foreseeable future, we're going to stick with 3.8.3. Though moving up to 4.x doesn't seem like it would affect many customers (repeating the same TS/sentry 6.x crossover search with TS 3.8 and 3.9 reveals a total of 5 projects), it does have the known side effect of replacing export statements which, after compilation, currently look like
`exports.SdkInfo = types_1.SdkInfo;`
with ones that look like
`Object.defineProperty(exports, "SdkInfo", { enumerable: true, get: function () { return types_1.SdkInfo; } });`.
(For those of you following along at home, that's a 3x character increase.) Though we might be able to engineer around this in order to avoid the inevitable substantial bundle size hit, attempting to do so is only worth it if there are features from 4.x that we desperately need. Given that we've agreed that right now there aren't, we'll stick with 3.8.3.
[1] https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export
[2] https://www.typescriptlang.org/tsconfig#isolatedModules
lobsterkatie
added a commit
that referenced
this pull request
Apr 26, 2022
This updates the SDK to use Typescript 3.8.3, in order to be able to use type-only imports and exports[1]. (These are needed for us to turn on `isolatedModules`, which is in turn is needed for us to switch our build tool away from `tsc`[2], since no other tool understands the relationship between files.)
As a result of this change, a few of the browser integration tests needed to be fixed so that all promises were explicitly awaited, a point about which the linter in 3.8 complains.
This is a breaking change for anyone using TS 3.7.x (there's no one using TS < 3.7.x, since that's our current minimum). That said, though there are plenty of public projects on GH using TS 3.7 and `@sentry/xyz`, if you restrict it to projects using TS 3.7 and `@sentry/xyz` 6.x, all you get are forks of this very repo. Granted, this isn't every project ever, but it's likely decently representative of the fact that if you've upgraded our SDK, you've almost certainly upgraded TS as well. We're going to wait until v7 to release this change in any case, but that's an indication that it won't affect many people when we do. (See this commit's PR for links to searches demonstrating the points.)
Note: Originally there was some thought of going farther, into TS 4.x, but we decided that for the foreseeable future, we're going to stick with 3.8.3. Though moving up to 4.x doesn't seem like it would affect many customers (repeating the same TS/sentry 6.x crossover search with TS 3.8 and 3.9 reveals a total of 5 projects), it does have the known side effect of replacing export statements which, after compilation, currently look like
`exports.SdkInfo = types_1.SdkInfo;`
with ones that look like
`Object.defineProperty(exports, "SdkInfo", { enumerable: true, get: function () { return types_1.SdkInfo; } });`.
(For those of you following along at home, that's a 3x character increase.) Though we might be able to engineer around this in order to avoid the inevitable substantial bundle size hit, attempting to do so is only worth it if there are features from 4.x that we desperately need. Given that we've agreed that right now there aren't, we'll stick with 3.8.3.
[1] https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export
[2] https://www.typescriptlang.org/tsconfig#isolatedModules
lobsterkatie
added a commit
that referenced
this pull request
May 10, 2022
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.
AbhiPrasad
pushed a commit
that referenced
this pull request
May 30, 2022
This updates the SDK to use Typescript 3.8.3, in order to be able to use type-only imports and exports[1]. (These are needed for us to turn on `isolatedModules`, which is in turn is needed for us to switch our build tool away from `tsc`[2], since no other tool understands the relationship between files.)
As a result of this change, a few of the browser integration tests needed to be fixed so that all promises were explicitly awaited, a point about which the linter in 3.8 complains.
This is a breaking change for anyone using TS 3.7.x (there's no one using TS < 3.7.x, since that's our current minimum). That said, though there are plenty of public projects on GH using TS 3.7 and `@sentry/xyz`, if you restrict it to projects using TS 3.7 and `@sentry/xyz` 6.x, all you get are forks of this very repo. Granted, this isn't every project ever, but it's likely decently representative of the fact that if you've upgraded our SDK, you've almost certainly upgraded TS as well. We're going to wait until v7 to release this change in any case, but that's an indication that it won't affect many people when we do. (See this commit's PR for links to searches demonstrating the points.)
Note: Originally there was some thought of going farther, into TS 4.x, but we decided that for the foreseeable future, we're going to stick with 3.8.3. Though moving up to 4.x doesn't seem like it would affect many customers (repeating the same TS/sentry 6.x crossover search with TS 3.8 and 3.9 reveals a total of 5 projects), it does have the known side effect of replacing export statements which, after compilation, currently look like
`exports.SdkInfo = types_1.SdkInfo;`
with ones that look like
`Object.defineProperty(exports, "SdkInfo", { enumerable: true, get: function () { return types_1.SdkInfo; } });`.
(For those of you following along at home, that's a 3x character increase.) Though we might be able to engineer around this in order to avoid the inevitable substantial bundle size hit, attempting to do so is only worth it if there are features from 4.x that we desperately need. Given that we've agreed that right now there aren't, we'll stick with 3.8.3.
[1] https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export
[2] https://www.typescriptlang.org/tsconfig#isolatedModules
AbhiPrasad
pushed a commit
that referenced
this pull request
May 30, 2022
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This is a repeat of #4491, to get it onto the main v7 branch.
This updates the SDK to use Typescript 3.8.3, in order to be able to use type-only imports and exports. (These are needed for us to turn on
isolatedModules, which is in turn is needed for us to switch our build tool away fromtsc, since no other tool understands the relationship between files.)As a result of this change, a few of the browser integration tests needed to be fixed so that all promises were explicitly awaited, a point about which the linter in 3.8 complains.
This is a breaking change for anyone using TS 3.7.x (there's no one using TS < 3.7.x, since that's our current minimum). That said, though there are plenty of public projects on GH using TS 3.7 and
@sentry/xyz, if you restrict it to projects using TS 3.7 and@sentry/xyz6.x, all you get are forks of this very repo. Granted, this isn't every project ever, but it's likely decently representative of the fact that if you've upgraded our SDK, you've almost certainly upgraded TS as well. We're going to wait until v7 to release this change in any case, but that's an indication that it won't affect many people when we do.Ultimately we may end up upgrading all the way to 4.x, but that can be left for a future PR if so.[UPDATE] For the foreseeable future, we're going to stick with 3.8.3. Though moving up to 4.x doesn't seem like it would affect many customers (repeating the same TS/sentry 6.x crossover search with TS 3.8 and 3.9 reveals a total of 5 projects), it does have the known side effect of replacing export statements which, after compilation, currently look likeexports.SdkInfo = types_1.SdkInfo;with ones that look likeObject.defineProperty(exports, "SdkInfo", { enumerable: true, get: function () { return types_1.SdkInfo; } });. (For those of you following along at home, that's a 3x character increase.) Though we might be able to engineer around this in order to avoid the inevitable substantial bundle size hit, attempting to do so is only worth it if there are features from 4.x that we desperately need. Given that we've agreed that right now there aren't, we'll stick with 3.8.3.