Skip to content

ref(ui): Update Sentry to js sdk 7 beta #34286

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

Merged
merged 9 commits into from
May 16, 2022
Merged

Conversation

k-fish
Copy link
Member

@k-fish k-fish commented May 5, 2022

Summary

This will let us try out the new beta sdk on Sentry itself to find out if we have any issues earlier and give feedback to the sdk team.

This will let us try out the new beta sdk on Sentry itself to find out if we have any issues earlier and give feedback to the sdk team.
@k-fish k-fish requested a review from a team as a code owner May 5, 2022 16:03
@k-fish k-fish requested a review from a team May 5, 2022 16:03
billyvg
billyvg previously approved these changes May 5, 2022
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

No breaking changes?

@smeubank
Copy link
Member

smeubank commented May 9, 2022

@billyvg there are breaking changes, but I am not sure which ones impact Sentry so please refer to the change log and let us know what you think of those as well. Since we also want to know if the way we communicate breaking changes is clear

@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2022

size-limit report 📦

Path Size
src/sentry/static/sentry/dist/entrypoints/app.js 62.26 KB (-0.04% 🔽)
src/sentry/static/sentry/dist/entrypoints/sentry.css 69.93 KB (0%)

@k-fish
Copy link
Member Author

k-fish commented May 10, 2022

@smeubank, I've gone over the migration guide w.r.t Sentry js and updated it now to fix the breaking change. The migration guide had pretty much all our breaking changes in it, there were only a couple notes.

The only thing still needing work is that it looks like we have a breaking change because our jest environment is using deprecated sdk api as well, so need to fix that, if someone from the sdk could confirm that child->startChild is correct? I don't see it in the guide. See the linked PR below.

Only other update for the migration guide is that maybe we could improve clarity for the BrowserClient changes since there are two sections referring to it. I was mistakenly looking at less-sync-api section for a minute before finding the explicit client options, might be worth adding a link in less-async-api if that's the same relation? I'm not 100% what the less-async-api section migration path is, seems a bit unclear in general since there is no action item.

@billyvg billyvg dismissed their stale review May 10, 2022 18:25

needs re-review

@billyvg
Copy link
Member

billyvg commented May 10, 2022

@smeubank have we considered providing codemods when making breaking changes?

@k-fish
Copy link
Member Author

k-fish commented May 10, 2022

@billyvg that's basically what I did for ours, although pretty quick rough code just for our changes. Re-writes via codemods would probably encourage adoption, for simple things like renames.

import { Transform, MemberExpression } from "jscodeshift";

const FullSeverityExpression: (level: string) => MemberExpression = (
  level
) => ({
  type: "MemberExpression",
  object: {
    type: "MemberExpression",
    object: {
      type: "Identifier",
      name: "Sentry",
    },
    property: {
      type: "Identifier",
      name: "Severity",
    },
  },
  property: {
    type: "Identifier",
    name: level,
  },
});

const PartialSeverityExpression: (level: string) => MemberExpression = (
  level
) => ({
  type: "MemberExpression",
  object: {
    type: "Identifier",
    name: "Severity",
  },
  property: {
    type: "Identifier",
    name: level,
  },
});

const transform: Transform = (file, api) => {
  const jscodeshift = api.jscodeshift;
  const ast = jscodeshift(file.source);
  let modified = false;

  ["Error", "Warning", "Info"].forEach((level) => {
    ast
      .find(jscodeshift.MemberExpression, FullSeverityExpression(level))
      .replaceWith(() => jscodeshift.identifier(`'${level.toLowerCase()}'`))
      .filter(() => (modified = true));
    ast
      .find(jscodeshift.MemberExpression, PartialSeverityExpression(level))
      .replaceWith(() => jscodeshift.identifier(`'${level.toLowerCase()}'`))
      .filter(() => (modified = true));
  });

  ast
    .find(jscodeshift.CallExpression, {
      callee: {
        object: {
          type: "Identifier",
          name: "Sentry",
        },
        property: {
          type: "Identifier",
          name: "init",
        },
      },
    })
    .forEach((path) => {
      // Sentry.init call.
      jscodeshift(path)
        .find(jscodeshift.Identifier, {
          type: "Identifier",
          name: "whitelistUrls",
        })
        .replaceWith(jscodeshift.identifier("allowUrls"))
        .filter(() => (modified = true));
    });

  if (modified) {
    return ast.toSource();
  } else {
    return null;
  }
};

@billyvg
Copy link
Member

billyvg commented May 10, 2022

@k-fish published 1.7.1 that fixed a syntax error

@smeubank
Copy link
Member

@smeubank have we considered providing codemods when making breaking changes?

codemods has come up but we did not do any work on it for this major release

@lforst
Copy link
Contributor

lforst commented May 11, 2022

if someone from the sdk could confirm that child->startChild is correct? I don't see it in the guide. See the linked PR below.

This looks kinda weird. I can't find any reference to child() in the SDK whatsoever - not even when going back to old versions. Is it possible that this doesn't reference SDK code at all? 🤔

I'm not 100% what the less-async-api section migration path is, seems a bit unclear in general since there is no action item.

The less-async-api section is part of the migration guide from 4.x -> 5.x/6.x. I sadly don't have much context to share here. Is this relevant for the v7 version bump? If it is, let me know and I'll investigate further!

We will discuss creating a code mod for the major - definitely an awesome suggestion.


Thank you so much for helping us dogfood this release 🙏 - really important to get some early feedback in as this is a huge update!

If there's anything else you need from the SDK team for getting this merged in, feel free to ping me :)

@billyvg
Copy link
Member

billyvg commented May 11, 2022

Ah need to apply this to getsentry too

@AbhiPrasad
Copy link
Member

We just released a new beta: https://www.npmjs.com/package/@sentry/browser/v/7.0.0-beta.1

This shouldn't change anything with the migration though (and has the LCP fixes merged in!)

@k-fish
Copy link
Member Author

k-fish commented May 13, 2022

Anyone want to give an approve?

@k-fish k-fish merged commit 619cdcb into master May 16, 2022
@k-fish k-fish deleted the ref/ui-update-sentry-7-beta branch May 16, 2022 16:07
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants