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

refactor(ui): use named functions for better tracing #12062

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Oct 21, 2023

Motivation

  • named functions are better for tracing since the name shows up in stack traces etc
    • instead of "anonymous"
    • similar to many other previous refactor PRs I made that do this as part of other refactors

Modifications

  • convert several functional components and plain functions to use named functions

    • instead of consts assigned to anonymous functions
  • still a good amount of these leftover, but I think I got over 50%+ now

    • got a little tired part way in 😅

Verification

these are stylistic changes outside of traces; they are otherwise semantically equivalent

- convert several functional components and plain functions to use named functions
  - instead of consts assigned to anonymous functions
  - named functions are better for tracing since the name shows up in stack traces etc
    - instead of "anonymous"

- still a good amount of these leftover, but I think I got over 50%+ now

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

still a good amount of these leftover, but I think I got over 50%+ now

Maybe cover 100% in the same PR?

@agilgur5
Copy link
Contributor Author

I can, but I stopped part-way in and this is still independently mergeable as is. No need to wait for no reason IMO

@terrytangyuan
Copy link
Member

I would appreciate a combined PR instead of tiny PRs with trivial changes.

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 22, 2023

I would appreciate a combined PR instead of tiny PRs with trivial changes.

What is the purpose of that? That seems like unnecessarily slowing down contributions. I don't see what benefit there is to that.
It'd be one thing if this was like 2 or 3 components, but this is like 30 of them; it's not a small number.

This is ~75 LoC of better tracing. I'd rather have that than not. Or wait indefinitely for ~150 LoC of better tracing. This was non-trivial to code (it is manual), and that's why it was tiring to do so and why I stopped part-way.

(to be clear, it is simple, no doubt, but it is very tedious. that's one of the reasons I hadn't done this earlier. especially as there are some rough edges due to very similar but semantically different syntaxes in modern JS (unfortunately) that require some more care -- I've intentionally left those out of this PR and only changed the exact same syntaxes here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants