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(browser): Replace TracekitStackFrame with Sentry StackFrame #4523

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Feb 9, 2022

This PR is built on top of #4514 so once that's merged these changes will be significantly reduced.
Before the above PR is merged, you can see the changes here

This PR:

  • Replaces all usages of TracekitStackFrame with the Sentry StackFrame and renames the fields appropriately.
  • Merge Opera regex tests in same function as others to remove duplication
  • Makes some simplifications because undefined is now used instead of null
  • Tightens up some Error types

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.

This is just about a 1% decrease in gzip + minified es5 bundle (even more in es6)! Slow and steady 🚶

packages/browser/src/parsers.ts Outdated Show resolved Hide resolved
packages/browser/src/tracekit.ts Show resolved Hide resolved
packages/browser/src/tracekit.ts Outdated Show resolved Hide resolved
packages/browser/src/tracekit.ts Outdated Show resolved Hide resolved
packages/browser/src/tracekit.ts Outdated Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Feb 10, 2022

This is just about a 1% decrease

I was honestly expecting this to increase bundle size since most of the fields are now longer!

@AbhiPrasad AbhiPrasad changed the title Remove TracekitStackFrame ref(browser): Replace TracekitStackFrame with Sentry StackFrame Feb 10, 2022
@AbhiPrasad AbhiPrasad added this to the Treeshaking / Bundle Size milestone Feb 10, 2022
@timfish timfish mentioned this pull request Feb 10, 2022
15 tasks
@timfish timfish marked this pull request as ready for review February 10, 2022 18:32
@timfish
Copy link
Collaborator Author

timfish commented Feb 10, 2022

Stripped out a few nulls and then realised quite a lot of code can be removed by combining the separate opera/other functions.

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.

Let's gooo!

Edit: there's a release ongoing, will merge this in as soon as it's cut. release is done

element = {
filename: parts[2],
function: parts[3] || UNKNOWN_FUNCTION,
lineno: +parts[1],
Copy link
Member

Choose a reason for hiding this comment

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

This is super byte size warrior, but perhaps to save on the repeated un-minifiable object props being defined when creating the element obj, we can extract into a function that takes args for each of these?

We do sacrifice readability of this file a little, and the savings might not justify it tbh. Thoughts?

// TS will complain about this
function createStackFrame(filename, function, lineno, colno?: StackFrame['colno']): StackFrame {
  const element = {
    filename,
    function,
    lineno,
  }

  if (colno) element.colno = colno;

  return element;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that would mean the fields would only need to be defined once all the parsers!

I'll add that to the next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a plan.

@AbhiPrasad AbhiPrasad merged commit 2c47a8b into getsentry:master Feb 11, 2022
@timfish timfish deleted the remove-tracekitstackframe branch February 11, 2022 17:10
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.

2 participants