Check if title should be updated

gaearon / react-document-title

Declarative, nested, stateful, isomorphic document.title for React

MIT License

1.86k stars 105 forks source link

Check if title should be updated #32

Closed buzinas closed 8 years ago

buzinas commented 8 years ago

This change prevents the document.title of being updated regardless if it had changed or not.

When using DocumentTitle today, any time some child component updates, the current document.title updates as well, and it shouldn't.

buzinas commented 8 years ago

@gaearon?

buzinas commented 8 years ago

Is anyone actively maintaining this project?

buzinas commented 8 years ago

@gaearon ...

gaearon commented 8 years ago

No, it's not maintained, please use react-helmet or equivalent. I should put up a badge about it.

buzinas commented 8 years ago

@gaearon Could you transfer it for me, or at least merge this PR? It's only an if statement.

There are two problems with react-helmet:

It's bloated for who only wants to change the document's title
It doesn't play well with Redux

gaearon commented 8 years ago

Can you explain this change better?

When using DocumentTitle today, any time some child component updates, the current document.title updates as well, and it shouldn't.

Why shouldn’t it? This sounds like the correct behavior.

I’m also not sure what exactly the check accomplishes. Is updating document.title expensive?

buzinas commented 8 years ago

@gaearon Sure, I can.

I'm developing a React/Redux Sports Betting application, and for each sport there is a "gamecast" feature, which simulates what's happening in a real game. I developed a SVG animation library for it, and it uses requestAnimationFrame and setState internally. But since my component is inside another component, that is inside another component (...) that is inside DocumentTitle, every frame the document.title is updated. It's not very expensive, but on my old mobile device, when I remove it, I can reach 45~50fps, and using it, I can only reach 30~35fps.

The check is about ~15x faster than updating every time (at least for those animations specifically, didn't test in other scenarios). The goal is to touch the DOM only when needed, since there is no need to update it if the value is the same as it was before.

gaearon commented 8 years ago

@buzinas

Added you as a collab on this repo and owner of npm package. Please feel free to release the fix when you’re ready. (Note my small nit in this PR.)

A few notes:

Please make sure to run npm run lint and npm test before releasing
Please either stick to ES5 or add a build system (there’s currently none)
Please make sure to follow semver

Thanks for your help!

buzinas commented 8 years ago

@gaearon Thanks very much for reopening the issue and also for the permissions.

I made the change you asked, and both linting and tests are passing (and I'm sticking to ES5, also).

About semver, should I realease this as a patch or minor bump? (Some people would call this a feature and bump minor version, others would call this a fix, and bump patch, so not sure what you prefer).

gaearon commented 8 years ago

I think this is safe to go as a patch.

buzinas commented 8 years ago

Thank you!

buzinas commented 8 years ago

Published v2.0.2, thanks very much!

gaearon commented 8 years ago

👍