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

Replace repetitive blurhash code with component #14267

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Replace repetitive blurhash code with component #14267

merged 1 commit into from
Jul 9, 2020

Conversation

brawaru
Copy link
Contributor

@brawaru brawaru commented Jul 8, 2020

This PR replaces all unnecessarily repeated code for decoding and embedding blurhash canvases with separate component — <Blurhash>.

Under the hood Blurhash component will use effect dependent on its props. This gives a few benefits: it will only be re-rendered whenever the hash or width/height/dummy props update, and will not render if canvas won't get to the final DOM, because then effect won't fire, which prevents weird bugs like #14257.

@brawaru
Copy link
Contributor Author

brawaru commented Jul 8, 2020

Oh, this one went trough the merge conflicts resolution on rebase, check twice if I didn't remove something important!

dummy = false,
...canvasProps
}) {
const canvasRef = /** @type {import('react').MutableRefObject<HTMLCanvasElement>} */ (useRef());
Copy link
Contributor Author

@brawaru brawaru Jul 8, 2020

Choose a reason for hiding this comment

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

Since I imported React, I could have replaced this one with React, welp not a big deal to spin up CI again 🤦

Suggested change
const canvasRef = /** @type {import('react').MutableRefObject<HTMLCanvasElement>} */ (useRef());
const canvasRef = /** @type {React.MutableRefObject<HTMLCanvasElement>} */ (useRef());

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Not a proper review yet but: having a component for this would definitely make sense, however I'm not sure how good an idea it is to use functional-components-but-with-API-hooks style since we aren't using it anywhere else.

Furthermore, I think useEffect gets called later than componentDidMount and componentDidUpdate, so it might result in a slight delay before the canvas being drawn.

@brawaru
Copy link
Contributor Author

brawaru commented Jul 8, 2020

@ThibG
however I'm not sure how good an idea it is to use functional-components-but-with-API-hooks style since we aren't using it anywhere else.

In my opinion functional components with hooks are easier to write, use and understand, unless they get too complex, in which case they can be split to more basic components and hooks abstracted under new hooks! The fact that many of Mastodon's components are written in class-like style doesn't mean all the new ones should too, especially those basic as this one.

In this example, if you would write this component using class, then you would have to, again: duplicate props checking, decoding calling and so on, here, however, you just useEffect which replaces need for two lifecycle methods (actually, useEffect may return a function which replaces componentDidUnmount), and has ability to check dependencies without you need to write if (prevProps newProps bla bla bla), you just pass an array and it does the job.

Furthermore, I think useEffect gets called later than componentDidMount and componentDidUpdate, so it might result in a slight delay before the canvas being drawn.

The only difference I may think of is that useEffect is asynchronous, other than this it is supposed to work just like componentDidMount and componentDidUpdate (?), I don't remember they had any difference in order they called. And if asynchronous call being a problem, there's always useLayoutEffect.

@ClearlyClaire
Copy link
Contributor

I'm not arguing that functional components with hooks are worse in any way. I think they're fine, it's just a pretty different style to Mastodon's codebase and thus contributors would need to understand both. I'll defer to @Gargron for this.

The only difference I may think of is that useEffect is asynchronous, other than this it is supposed to work just like componentDidMount and componentDidUpdate (?), I don't remember they had any difference in order they called. And if asynchronous call being a problem, there's always useLayoutEffect.

According to https://en.reactjs.org/docs/hooks-reference.html#timing-of-effects:

Unlike componentDidMount and componentDidUpdate, the function passed to useEffect fires after layout and paint, during a deferred event. This makes it suitable for the many common side effects, like setting up subscriptions and event handlers, because most types of work shouldn’t block the browser from updating the screen.

useLayoutEffect could be more suitable indeed… though maybe we don't actually care if the blurhash isn't drawn immediately, idk

@brawaru
Copy link
Contributor Author

brawaru commented Jul 8, 2020

it's just a pretty different style to Mastodon's codebase and thus contributors would need to understand both

That's why I think we should look into writing more new functional components with hooks and maybe rewriting some old new ones to also check for the bugs, so it won't be something extraordinary to see.

Talking about contributors

It's already really hard to wrap head around Mastodon code. When you see a functional component you might think “What kind of magic is happening there?”, you'll google a few methods that you don't know about, get to React docs, and it will teach you pretty much everything you needed to know to understand for the most part what this component is doing. You cannot really have this with classes, you have to learn of specific things like binding, lifecycle methods and such.

The benefit for contributors I think, is that is written in a readable manner, in a single function (maybe a few split functions), with understandable names (const onClick = useCallback, for example). It is probably much easier to maintain and change. Should note, however, such change won't help much contributors if we won't solve different problems with code, such as formatting (there is no styleguide and overall it's a disaster), code conventions and documentation… It's for another discussion.

though maybe we don't actually care if the blurhash isn't drawn immediately, idk

No flashes or whatsoever were seen when running on developer instance, even with CPU slowdown. I guess it's fine, if someone reports anything unusual we may look into it and change it to useLayoutEffect.


useEffect(() => {
const { current: canvas } = canvasRef;
canvas.width = canvas.width; // resets canvas
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this line about, exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's HTML magic, whenever you use width/height setters on canvas, it will be completely reset, even if value does not change. It's used here in case (probably never will be the case, though) dummy prop changes, therefore canvas will be reset and left blank because of the return at the next line.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I can't comment on the type annotations, as I don't know them, but otherwise this PR looks fine, provided @Gargron is ok with the functional-with-hooks style.

Copy link
Member

@ykzts ykzts left a comment

Choose a reason for hiding this comment

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

I think functional components with hooks that are simple to write are good.

In addition, I confirmed the type annotations.

This commit replaces all unnecessarily repeated code for decoding and
embedding blurhash canvases with separate component - <Blurhash>.

Under the hood Blurhash component will use effect dependent on its
props. This gives a few benefits: it will only be re-rendered whenever
the hash or width/height/dummy props update, and will not render if
canvas won't get to the final DOM, because then effect won't fire,
which prevents weird bugs like #14257.
@Gargron Gargron merged commit 61c07c3 into mastodon:master Jul 9, 2020
ClearlyClaire pushed a commit to ClearlyClaire/mastodon that referenced this pull request Jul 10, 2020
)

Port 61c07c3 to glitch-soc

Signed-off-by: Thibaut Girka <thib@sitedethib.com>
abcang pushed a commit to abcang/mastodon that referenced this pull request Jul 11, 2020
This commit replaces all unnecessarily repeated code for decoding and
embedding blurhash canvases with separate component - <Blurhash>.

Under the hood Blurhash component will use effect dependent on its
props. This gives a few benefits: it will only be re-rendered whenever
the hash or width/height/dummy props update, and will not render if
canvas won't get to the final DOM, because then effect won't fire,
which prevents weird bugs like mastodon#14257.
shouo1987 pushed a commit to CrossGate-Pawoo/mastodon that referenced this pull request Dec 7, 2022
This commit replaces all unnecessarily repeated code for decoding and
embedding blurhash canvases with separate component - <Blurhash>.

Under the hood Blurhash component will use effect dependent on its
props. This gives a few benefits: it will only be re-rendered whenever
the hash or width/height/dummy props update, and will not render if
canvas won't get to the final DOM, because then effect won't fire,
which prevents weird bugs like mastodon#14257.
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.

4 participants