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 pure-render-decorator with React.PureComponent #386

Closed
wants to merge 2 commits into from

Conversation

jxodwyer
Copy link

@jxodwyer jxodwyer commented Dec 22, 2016

Fixes #144

Changes proposed in this pull request:

  • Replace all uses of @PureRender decorator with React.PureComponent
  • Create AbstractPureComponent in order to cover components that were leveraging both @PureRender and AbstractComponent. One example of this

Reviewers should focus on:

  • The creation of the AbstractPureComponent. Especially in regards to imports like this

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

i'm onboard for this change but it's a 🔥 serious API break 🔥 as it will require recent versions of React that provides PureComponent.

our current React support story is "^15.0.1 || ^0.14" and it's unlikely we'll be able to commit to ^15.3 in the near future because of the wide variety of internal apps using older versions.

* A pure abstract component that Blueprint components can extend
* in order to add some common functionality like runtime props validation.
*/
export abstract class AbstractPureComponent<P, S> extends React.PureComponent<P, S> {
Copy link
Contributor

Choose a reason for hiding this comment

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

there must be a way to reuse the code from AbstractComponent instead of duplicating it here. maybe this is the impetus to refactor AC into separate validation and timeout controllers.

import * as React from "react";

import { AbstractComponent } from "../../common/abstractComponent";
import { AbstractPureComponent } from "../../common/abstractPureComponent";
Copy link
Contributor

Choose a reason for hiding this comment

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

leave the blank line above. it separates import groups so they can be alphabetized separately.

@giladgray giladgray added this to the 2.x milestone Jan 9, 2017
@giladgray
Copy link
Contributor

@jxodwyer thank you for implementing this feature! unfortunately we're not ready to accept it as it's a significant API break (requiring React >= 15.3) that we can't commit to yet.

I've added it to the 2.x milestone so we can revisit when it's time to break things, but I'm sure the merge conflicts will only get worse. We'll check back in soon.

@giladgray giladgray closed this Jan 9, 2017
@jxodwyer
Copy link
Author

jxodwyer commented Jan 9, 2017

Sounds good @giladgray Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants