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

selectorFactory: pass ownProps to areStatesEqual #1947

Closed
wants to merge 1 commit into from

Conversation

jspurlin
Copy link
Contributor

This PR adds the ability for consumers to have access to ownProps inside of areStatesEqual.

Currently there is no way to

See: #781

@jspurlin
Copy link
Contributor Author

Note, I'm also willing to make the corresponding changes in the v7 and master branches

@phryneas
Copy link
Member

phryneas commented Aug 29, 2022

Our general stance on connect is that it is a legacy api that's essentially mostly there to support class components, which themselves are pretty much a legacy api in the React ecosystem - and that as such we won't be adding any changes or new features to connect since we generally want to encourage people to slowly migrate away from it instead of sending the opposite message.

Generally we want people to use the hooks as they work much better with TypeScript and are a lot more straightforward to teach, write and read.

Back in 2020, when #781 was last discussed, that wasn't as clear as today yet.

@jspurlin
Copy link
Contributor Author

So the approach before was "We don't know if we want to add the functionality, push it off" to "Hmm, we've gotten multiple requests for this, it seems reasonable to add especially considering the straightforwardness of the change" to "We pushed it off long enough that now it's considered legacy and we don't want to allow the change"? That doesn't sound great.

While I agree that migrating to the hooks approach is the better long-term strategy, making that transition might require a substantial amount of work and enabling the functionality this PR brings helps bridge the gap.

@phryneas
Copy link
Member

I see it this way, the other maintainers might chime in with different opinions:

If you need this feature now, the code using connect has not been written yet. At this point in time, you are better off doing the same in a hook. Even if you are using a class component, you can still achieve this without adding another feature to connect by simply wrapping said class component into a function component and using the hooks there.

Also, the whole situation of "I have a class component and now want to add connect" is very questionable. It's more likely that you had a class component with connect for a long time, working without this feature - or that you are writing a new class component right now, which you really should avoid if you want it to work well with modern React features (and modern libraries) going to the future.

Writing new code with connect (or classes) means knowingly introducing technical debt into your project. I really can only recommend against doing that.

@timdorr
Copy link
Member

timdorr commented Aug 29, 2022

One point of order: This is targetting v5, which was last released almost 3 years ago. I'd rather not touch something that old. This should probably target v7 instead.

I would say that we should add this in. I agree that it may not be best for greenfield code to use connect. But if you're working in an existing project structure that is designed around building connected class components (and with a team that is trained as such), you're probably still going to use them for new code you write. So, adding this in won't be a concession in getting folks to use hooks. They aren't switching anytime soon.

@jspurlin
Copy link
Contributor Author

The assumption that

If you need this feature now, the code using connect has not been written yet.

may not be correct (and is not correct in my case). We have many connected components and when investigating application performance, it was determined that mapStateToProps (and the resulting code that comes along running mapStateToProps) is a performance bottleneck. There are straightforward ways to mitigate the need for running mapStateToProps (and the rest of the update flow) from areStatesEqual if we have access to ownProps (like ownProps.id, for example)

@jspurlin
Copy link
Contributor Author

Is there any opposition to adding this to v5 for backwards compat reasons?

@phryneas
Copy link
Member

phryneas commented Aug 29, 2022

Is there any opposition to adding this to v5 for backwards compat reasons?

Please at least update your stack if you want new features. 🙁

There's even a good chance that you won't be facing your current performance problems with an up-to-date stack.

@markerikson
Copy link
Contributor

markerikson commented Aug 29, 2022

I'm willing to look at this for v7 and v8.

But we're no longer actively maintaining v5 and v6 at this point, and if for some reason your app is still on v5, I would strongly urge you to upgrade to v7 immediately. The public API is the same (with just a minor change to how the refs option works), and it has better perf.

That said, I'm also very curious to know more about the perf issues you're dealing with - any chance you could record a Chrome JS perf capture of these issues, zip the JSON, and either attach it to an issue or send it to us via Discord?

@jspurlin
Copy link
Contributor Author

I've opened #1951 against master and #1952 against v7

@timdorr
Copy link
Member

timdorr commented Aug 30, 2022

OK, closing this one out.

@timdorr timdorr closed this Aug 30, 2022
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