-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Delete old transports #4967
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
Conversation
3436687
to
3051cad
Compare
size-limit report 📦
|
bafbcfb
to
817a118
Compare
817a118
to
4e64c8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
// assert on what the class provides and what it leaves to the concrete class to implement | ||
class SimpleTransport extends BaseTransport {} | ||
|
||
// TODO(v7): Re-enable these tests with client reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we delete this we might forget to re add tests. Can we add a note somewhere (maybe in the BrowserClient tests) to add those client report tests back in?
/** | ||
* Creates a Transport that uses the Fetch API to send events to Sentry. | ||
*/ | ||
export function makeNewFetchTransport( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add a TODO here to rename this function? Same for xhr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename
packages/node/src/transports/http.ts
Outdated
// - Move this file one folder upwards | ||
// - Delete "transports" folder | ||
// OR | ||
// - Split this file up and leave it in the transports folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this todo now.
rename the dist directories in all build dirs to cjs. Hence, also the tarballs' structure changes which is why this PR also introduces a breaking change. Additional change: cleanup `yarn clean` commands by removing no longer existing directories
This updates `jest`, `ts-jest`, and `jest-environment-node` to the latest versions, in order to facilitate code transformations during `ts-jest`'s on-the-fly compilation that will become necessary once we move to ES6. (More detail on this to come in the PR which actually introduces said transformation, but TL;DR the way we use and extend `global` is fine if it's a `var` (which it is in ES5 Land) but less fine if it's a `const` (which it becomes under ES6), and we need to fix that for tests to run.) It also updates `jsdom`. Together these updates meant that a larger number of packages needed to be downgraded in order for tests to run in node 8 and 10. This therefore also reworks the test script a bit to account for those changes. Finally, this removes the test environment from our main jest config, as its value has become the default in latest version of jest.
As part of the new build process, a fair amount of new rollup-related code is going to be added. To keep things from getting too unwieldy, this splits the existing code up into modules. It also makes two other small changes, one for consistency and one to differentiate the current rollup code (which is for building bundles) from the future rollup code (which will be for building npm packages): - All plugins are now generated through factory functions (`makeXXXPlugin`). - Both the `makeConfigVariants` function and the individual `rollup.config.js` files have been renamed to make it clear they're for creating bundles. For now all of the resulting modules live in a `rollup` folder at the top level of the repo. In the long run, these would be good candidates to go into an `@sentry-internal/dev-utils` package.
Delete old transports as we've switched entirely to new v7 transports.
5ad27a4
to
dac51b7
Compare
Delete old transports as we've switched entirely to new v7 transports.
Delete old transports as we've switched entirely to new v7 transports.
Delete old transports as we've switched entirely to new v7 transports.
Following up on work in #4943
Delete old transports as we've switched entirely to new v7 transports.
Resolves https://getsentry.atlassian.net/browse/WEB-776