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

Raven-js likes to share, let's make it more introverted. #124

Closed
wants to merge 1 commit into from

Conversation

hamishcampbell
Copy link

  • All functions (e.g. 'private' ones, like each) moved into the
    Raven global to prevent them polluting the window object. Misc
    variables too (cachedAuth etc).
  • Added hitch to apply scope to callbacks, primarily
    so that each can recieve callbacks with scope set. each
    now applies this instead of null.
  • Moved global* variables into Raven object properties - since
    Raven is global anyway. Included legacy support for loading
    but possible issues if you're running multiple Raven versions
    and changing global settings after it's loaded.
  • Refactored for cleanliness:
    • Remove one line control structures (inconsistently applied
      and linters don't like them)
    • Declare functions nicely (wrapped, success, etc) so the
      test runner doesn't complain about strict violations
    • Use function level strict directive so sources can be concatenated
      and strict behaviour maintained.
    • Wrap it all up, put a bow on it.
  • Update tests.

  - All functions (e.g. 'private' ones, like `each`) moved into the
    Raven global to prevent them polluting the window object. Misc
    variables too (`cachedAuth` etc).
  - Added `hitch` to apply scope to callbacks, primarily
    so that `each` can recieve callbacks with scope set. `each`
    now applies `this` instead of `null`.
  - Moved global* variables into Raven object properties - since
    Raven is global anyway. Included legacy support for loading
    but possible issues if you're running multiple Raven versions
    and changing global settings after it's loaded.
  - Refactored for cleanliness:
     - Remove one line control structures (inconsistently applied
       and linters don't like them)
     - Declare functions nicely (`wrapped`, `success`, etc) so the
       test runner doesn't complain about strict violations
     - Wrap it all up, put a bow on it.
  - Update tests.
@mattrobenolt
Copy link
Contributor

Umm, not quite sure what all is going on here because I'm distracted by the noise of moving everythign into the Raven object, which is wrong.

When compiling the script, a header and footer are applied making a closure out of the entire script, not exposing anything except Raven.

What you did also makes the compressed file size much larger since the names of the functions can't be munged smaller.

If you want to resubmit, I'd be happy to look at the other things.

Also, try and keep pull requests as simple as possible. One pull request per idea. If, for example, I agreed on some but not all of the things, it's harder to cherry-pick just the things we both agree on. :)

@hamishcampbell
Copy link
Author

Erwoops. Should've looked before leaping.

In that case, my only issue is it would be useful to have a build command that only produced the concatenated (un uglified) source so I can let the dojo build system handle that side of it.

@mattrobenolt
Copy link
Contributor

So make raven gives you a minified and unminified version. I also distribute builds that are unminifed in the repository. Is that not enough?

mydea added a commit that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)
mydea added a commit that referenced this pull request Oct 31, 2023
- feat: Export getCanvasManager & allow passing it to record()
[#122](getsentry/rrweb#122)
- feat: Remove hooks related code, which is not used
[#126](getsentry/rrweb#126)
- feat: Remove plugins related code, which is not used
[#123](getsentry/rrweb#123)
- feat: Refactor module scope vars & export mirror & `takeFullSnapshot`
directly [#113](getsentry/rrweb#113)
- fix(rrweb): Fix rule.style being undefined
[#121](getsentry/rrweb#121)
- ref: Avoid unnecessary cloning of objects or arrays
[#125](getsentry/rrweb#125)
- ref: Avoid cloning events to add timestamp
[#124](getsentry/rrweb#124)


Note: With this update, canvas is _always_ excluded, unless we opt in by
passing a `getCanvasManager` function to `record()`. We'll provide a way
to do this once we have a fully formed canvas story. For now, this will
reduce bundle size considerably for all SDK users.
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