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

React strict mode bug #382

Open
catamphetamine opened this issue Mar 24, 2023 · 5 comments
Open

React strict mode bug #382

catamphetamine opened this issue Mar 24, 2023 · 5 comments

Comments

@catamphetamine
Copy link

catamphetamine commented Mar 24, 2023

A scroll-behavior instance is created in the constructor() of the <ScrollManager/> component.
According to React strict mode docs, that's an anti-pattern that results in incorrect operation.

  • Component instance A is created
  • Component instance B is created
  • componentDidMount() is called with this being B
  • componentWillUnmount() is called with this being B
  • componentDidMount() is called with this being B

Because of that, clean-up of Component instance A is not run and a scroll event listener with stale props (initial location) responds to scroll events forever.

A fix would be creating a scroll-behavior instance in componentDidMount().

I also rewrote <ScrollManager/> in React hooks in case you're interested:
https://github.com/catamphetamine/react-pages/blob/master/lib/router/client/found-scroll/ScrollManager.js
(it also contains the fix for the issue)
(I don't guarantee the correctness of the rewrite though)

@catamphetamine catamphetamine changed the title React strict mode React strict mode bug Mar 24, 2023
@rosskevin
Copy link

@catamphetamine is there a reason you haven't PR'd your hooks fix to this repo?

@catamphetamine
Copy link
Author

@rosskevin I guess the reason is that I completely rewrote the component and that would be not a small PR as a result. More like complete file replacement rather than a small diff of it. So PR or not PR, it would be easier to just copy-and-paste the file by replacing the old one in this library. PRs are mostly for smaller diffs.

@rosskevin
Copy link

rosskevin commented Jun 28, 2023

Perhaps you misunderstand. PRs are for improvements. This is an improvement and I for one would like to use it. I do not want to copy/paste improvements of code from the ecosystem into my source tree, I want to share the burden of maintenance/improvements with the rest of the world via the original repos/dependencies.

I looked where you put the code and it seems like you are trying to copy/paste all kinds of dependencies into your repo but not give back to the sources. I don't think you are heading down a path that is sustainable.

@catamphetamine
Copy link
Author

Perhaps you misunderstand.

Nah, I don't.

PRs are for improvements.

Yes.

This is an improvement and I for one would like to use it.

And you can do so by copy-pasting the file to your project.

I want to share the burden

Your choice. I'll pass.

I looked where you put the code and it seems like you are trying to copy/paste all kinds of dependencies into your repo but not give back to the sources.

I wonder how making the source codes publicly available on the Internet leads to "not give back to the sources".

I don't think you are heading down a path that is sustainable.

So fixing issues of an open-source repo and then showing the fix to the maintainers is somehow non-sustainable to a casual passer on the Internet. Ok.

@catamphetamine
Copy link
Author

catamphetamine commented Jun 28, 2023

@rosskevin I dunno if GitHub still notifies a user when someone from the blacklist messages them. In any case, I've blocked you due to the apparent non-constructive/aggressive behavior in your comments.

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

No branches or pull requests

2 participants