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(hub): Move @sentry/hub code to @sentry/core #5823

Merged
merged 9 commits into from
Oct 7, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 26, 2022

Closes #5665

This PR:

  • Moves all of the @sentry/hub code files to @sentry/core
  • Changes @sentry/hub to a stub that re-exports the same types from @sentry/core
    • @sentry/hub now only depends on @sentry/core
  • Switches internal imports @sentry/hub -> @sentry/core
    • Removes @sentry/hub as a dependency from packages
  • Fixes all the mocks/spys/imports in tests
  • Leaves the @sentry/hub tests where they are for now

Surprisingly this results in a 159 byte increase in minified bundle size!

I've compared the type exports from @sentry/hub from before and after this PR and the exports match so it looks like this isn't a breaking change 😬

@timfish timfish changed the title ref(hub): Move @sentry/hub code to @sentry/core ref(hub): Move @sentry/hub code to @sentry/core Sep 26, 2022
@timfish

This comment was marked as outdated.

@AbhiPrasad
Copy link
Member

We merged in #5873, which prob requires a more tricky rebase.

@timfish
Copy link
Collaborator Author

timfish commented Oct 5, 2022

Should we be marking all @sentry/hub exports as Deprecated - Moved to @sentry/core. @sentry/hub will be removed in v8?

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

For future readers: We'll save deleting @sentry/hub for the next major version, this just sets us up to do further refactors to work toward truly platform agnostic packages.

@AbhiPrasad
Copy link
Member

Should we be marking all @sentry/hub exports as Deprecated - Moved to @sentry/core. @sentry/hub will be removed in v8?

Good idea! Let's do that.

@timfish
Copy link
Collaborator Author

timfish commented Oct 5, 2022

Since the exports in @sentry/hub are just re-exports of external types, the only way to mark them as deprecated is to re-declare them as const and then mark with jsdoc like:

export { getCurrentHub as getCurrentHubCore } from '@sentry/core';

/**
 * @deprecated This export has moved to @sentry/core. The @sentry/hub package will be removed in v8 
 */
const getCurrentHub = getCurrentHubCore;

export { getCurrentHub }

@timfish
Copy link
Collaborator Author

timfish commented Oct 5, 2022

Ok, I think this is now good to go.

I couldn't mark Scope and Hub as deprecated like above because setting them as a const turns them into a value that can't then be used as a type!

@lobsterkatie
Copy link
Member

lobsterkatie commented Oct 7, 2022

I couldn't mark Scope and Hub as deprecated like above because setting them as a const turns them into a value that can't then be used as a type!

@timfish, since they're classes, could you do

import { Hub as CoreHub } from '@sentry/core';

/**
 * @deprecated This export has moved to @sentry/core. The @sentry/hub package will be removed in v8 
 */
export class Hub extends CoreHub { };

?

@timfish
Copy link
Collaborator Author

timfish commented Oct 7, 2022

It does leave me with a strange feeling that it is somehow exporting a different type to before but my logical side says that it's totally fine! 🙃

@AbhiPrasad AbhiPrasad merged commit 8570f0d into getsentry:master Oct 7, 2022
@AbhiPrasad
Copy link
Member

Can we move the tests in another PR?

Let's try to get this released on Monday alongside the NextJS experimental flag switch

@timfish
Copy link
Collaborator Author

timfish commented Oct 7, 2022

Leaving the tests in hub was instrumental in ensuring that there were no breaking changes in @sentry/hub exports.

Safe to move then now?

@AbhiPrasad
Copy link
Member

Leaving the tests in hub was instrumental in ensuring that there were no breaking changes in @sentry/hub exports.

Ahhh great point, then let's keep them until we do the actual deletion during the major.

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.

[v8] Remove @sentry/hub package
3 participants