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

Client-side JS performance optimizations #9272

Merged
merged 9 commits into from
May 20, 2024
Merged

Conversation

jimrandomh
Copy link
Collaborator

@jimrandomh jimrandomh commented May 13, 2024

Introduced in: f157ea3

Due to an inverted condition, if the user is logged out, we would update
the user's timezone context and cookie even if it wasn't changed.

was rendering its (conceptual) children via a
referentially-unstable wrapper component, so any rerenders propagating
from above would cause a rerender-from-scratch with loss of state. One
of the places this is used is QuickTakesSection, which contains a
CkEditor comment editor. What this meant in practice is that on load,
CkEditor would initialize twice on the client, duplicating a bunch of
plugin-initialization work.

The way wrapCkEditor was written, you have to import (and trigger some
plugin initialization for) every version of the editor (comment, post,
and collaborative), regardless of which you're going to use.

Make it so that you only set up the config for the one you're going to
use, and memoize the config initialization.

This prevents downstream rerenders on pageload, saving some JS execution
time.

┆Issue is synchronized with this Asana task by Unito

Introduced in: f157ea3

Due to an inverted condition, if the user is logged out, we would update
the user's timezone context and cookie even if it wasn't changed.
<ExpandableSection> was rendering its (conceptual) children via a
referentially-unstable wrapper component, so any rerenders propagating
from above would cause a rerender-from-scratch with loss of state. One
of the places this is used is QuickTakesSection, which contains a
CkEditor comment editor. What this meant in practice is that on load,
CkEditor would initialize twice on the client, duplicating a bunch of
plugin-initialization work.
The way wrapCkEditor was written, you have to import (and trigger some
plugin initialization for) every version of the editor (comment, post,
and collaborative), regardless of which you're going to use.

Make it so that you only set up the config for the one you're going to
use, and memoize the config initialization.
This prevents downstream rerenders on pageload, saving some JS execution
time.
@jimrandomh jimrandomh requested a review from a team as a code owner May 13, 2024 01:39
@jimrandomh jimrandomh requested review from Will-Howard and removed request for a team May 13, 2024 01:39
import { Components, registerComponent } from "../../lib/vulcan-lib";
import PropTypes from 'prop-types';

const styles = (theme: ThemeType): JssStyles => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid returning JssStyles, so that classes can be typed classes: ClassesType<typeof styles> (even though there aren't any styles in this case)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, this file wasn't supposed to be in this commit at all (it was hanging out in my working directory from an unrelated thing)

Copy link
Collaborator

@Will-Howard Will-Howard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@jpaddison3 jpaddison3 left a comment

Choose a reason for hiding this comment

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

(@Will-Howard I think you forgot to make your review an approving one)

@jimrandomh jimrandomh merged commit f96dc22 into master May 20, 2024
18 checks passed
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.

None yet

4 participants