Add Pure Render Decorators #4

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@taion
Contributor

taion commented May 13, 2015

For gaearon/react-pure-render#2

Should this be @PureDecorator instead?

@gaearon

View changes

.gitignore
+/index.js
+/mixin.js
+/shallowEqual.js

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

Why /?

@gaearon

gaearon May 14, 2015

Owner

Why /?

This comment has been minimized.

@taion

taion May 14, 2015

Contributor

Otherwise it picks up the files under src/. I could add -f, but this seems better.

@taion

taion May 14, 2015

Contributor

Otherwise it picks up the files under src/. I could add -f, but this seems better.

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

Maybe ./ works? / seems kinda weird

@gaearon

gaearon May 14, 2015

Owner

Maybe ./ works? / seems kinda weird

This comment has been minimized.

@taion

taion May 14, 2015

Contributor

This pattern is explicitly documented in the Git book: http://git-scm.com/docs/gitignore

A leading slash matches the beginning of the pathname. For example, "/*.c" matches "cat-file.c" but not "mozilla-sha1/sha1.c".

And ./ doesn't work - I just checked.

@taion

taion May 14, 2015

Contributor

This pattern is explicitly documented in the Git book: http://git-scm.com/docs/gitignore

A leading slash matches the beginning of the pathname. For example, "/*.c" matches "cat-file.c" but not "mozilla-sha1/sha1.c".

And ./ doesn't work - I just checked.

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

Oh I see. Ok!

@gaearon

gaearon May 14, 2015

Owner

Oh I see. Ok!

@gaearon

View changes

src/decorator.js
+import { Component } from 'react';
+
+export default function PureRender(Component) {
+ const componentName = Component.name || 'component';

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

Please make it Component.displayName || Component.name || 'component'

@gaearon

gaearon May 14, 2015

Owner

Please make it Component.displayName || Component.name || 'component'

This comment has been minimized.

@taion

taion May 14, 2015

Contributor

Will do.

@taion

taion May 14, 2015

Contributor

Will do.

@gaearon

View changes

src/decorator.js
+ const componentName = Component.name || 'component';
+
+ if (!(Component.prototype instanceof Component)) {
+ throw new Error(`${componentName} is not a React component`);

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

This is not necessary. You don't have to inherit from Component for it to work. Maybe replace this with a simple check for prototype's existance?

@gaearon

gaearon May 14, 2015

Owner

This is not necessary. You don't have to inherit from Component for it to work. Maybe replace this with a simple check for prototype's existance?

This comment has been minimized.

@taion

taion May 14, 2015

Contributor

Why not? Don't all React components have to inherit from Component? It wouldn't make sense to apply this to an ES5-style React component.

@taion

taion May 14, 2015

Contributor

Why not? Don't all React components have to inherit from Component? It wouldn't make sense to apply this to an ES5-style React component.

This comment has been minimized.

@gaearon

gaearon May 14, 2015

Owner

Don't all React components have to inherit from Component?

Nope. You can totally not inherit, as long as you don't need forceUpdate or setState. I do it all the time.

Also, why not allow it on createClass-style components? It should work, no?

@gaearon

gaearon May 14, 2015

Owner

Don't all React components have to inherit from Component?

Nope. You can totally not inherit, as long as you don't need forceUpdate or setState. I do it all the time.

Also, why not allow it on createClass-style components? It should work, no?

This comment has been minimized.

@taion

taion May 14, 2015

Contributor

Huh. I didn't realize you could do that. Cool! Okay, will make this change too.

@taion

taion May 14, 2015

Contributor

Huh. I didn't realize you could do that. Cool! Okay, will make this change too.

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 14, 2015

Contributor

Do you want me to squash these fixes, or would you prefer a separate commit?

Contributor

taion commented May 14, 2015

Do you want me to squash these fixes, or would you prefer a separate commit?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 14, 2015

Owner

Let's squash

Owner

gaearon commented May 14, 2015

Let's squash

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 14, 2015

Owner

Now that I think of it, there's another approach I like more. Decorating render function. The decorator would return memoized result of props, state and context are the same.

Can you look into making that work?

Owner

gaearon commented May 14, 2015

Now that I think of it, there's another approach I like more. Decorating render function. The decorator would return memoized result of props, state and context are the same.

Can you look into making that work?

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 14, 2015

Contributor

Huh. Cool! I can take a look at that over the weekend (on a separate PR). Need to look into whether I need to make (shallow) copies of the old props/state objects to make that work.

What are your thoughts regarding naming? Should I rename this to e.g. @pureClass? And call the latter @pureMethod?

Contributor

taion commented May 14, 2015

Huh. Cool! I can take a look at that over the weekend (on a separate PR). Need to look into whether I need to make (shallow) copies of the old props/state objects to make that work.

What are your thoughts regarding naming? Should I rename this to e.g. @pureClass? And call the latter @pureMethod?

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 14, 2015

Contributor

I incorporated the requested changes.

The new prototype check won't prevent someone from using this to decorate a function or something. Then again, someone decorating something other than a class with @pureClass probably deserves whatever they were getting, anyway.

Contributor

taion commented May 14, 2015

I incorporated the requested changes.

The new prototype check won't prevent someone from using this to decorate a function or something. Then again, someone decorating something other than a class with @pureClass probably deserves whatever they were getting, anyway.

@taion taion changed the title from Add @PureRender decorator to Add @pureClass decorator May 14, 2015

@taion taion changed the title from Add @pureClass decorator to Add Pure Render Decorators May 16, 2015

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 16, 2015

Contributor

I added @pureMethod. Two questions:

  1. I forward arguments. This isn't necessary for #render(), but might be nice for other methods. If I'm not mistaken, this will inflict a performance hit on most VMs. Do you think this is worth keeping?
  2. Cursory testing indicates that I don't need to make a copy of the old state and props. Do you think I should make a shallow copy just to be safe?
Contributor

taion commented May 16, 2015

I added @pureMethod. Two questions:

  1. I forward arguments. This isn't necessary for #render(), but might be nice for other methods. If I'm not mistaken, this will inflict a performance hit on most VMs. Do you think this is worth keeping?
  2. Cursory testing indicates that I don't need to make a copy of the old state and props. Do you think I should make a shallow copy just to be safe?
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 18, 2015

Owner

I'm very busy in the beginning of this week so I'm going to take a look a bit later, sorry!

Owner

gaearon commented May 18, 2015

I'm very busy in the beginning of this week so I'm going to take a look a bit later, sorry!

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 18, 2015

Contributor

No worries. In addition to the 2 questions above, it just occurred to me that I could merge @pureClass and @pureMethod by just checking the signature with which the decorator was called. Let me know if you think that'd be preferable (I don't think it would be, though, since it's much less explicit and for no good reason).

Contributor

taion commented May 18, 2015

No worries. In addition to the 2 questions above, it just occurred to me that I could merge @pureClass and @pureMethod by just checking the signature with which the decorator was called. Let me know if you think that'd be preferable (I don't think it would be, though, since it's much less explicit and for no good reason).

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 27, 2015

Contributor

Did you get a chance to look at this?

Contributor

taion commented May 27, 2015

Did you get a chance to look at this?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 27, 2015

Owner

@taion Not yet. I might be able in a week or two. Currently busy with a talk I'm giving tomorrow. :-)

Owner

gaearon commented May 27, 2015

@taion Not yet. I might be able in a week or two. Currently busy with a talk I'm giving tomorrow. :-)

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion May 27, 2015

Contributor

Oh, right. (: Good luck!

Contributor

taion commented May 27, 2015

Oh, right. (: Good luck!

@axyz

This comment has been minimized.

Show comment
Hide comment
@axyz

axyz Jul 21, 2015

any news about that?

axyz commented Jul 21, 2015

any news about that?

@michaelcontento

This comment has been minimized.

Show comment
Hide comment
@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Aug 28, 2015

Contributor

Ping!

The @pureMethod decorator is hackier than I'd like, but I think people would find @pureClass to be helpful.

Contributor

taion commented Aug 28, 2015

Ping!

The @pureMethod decorator is hackier than I'd like, but I think people would find @pureClass to be helpful.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 28, 2015

Owner

No news yet. I just released Redux 1.0 a few weeks ago and has been busy with React Hot Loader 2 recently which needs a lot of work and has much more priority. When I'm idle, I'll look at this, but for now, feel free to release this separately if I'm being frustrating. Sorry!

Owner

gaearon commented Aug 28, 2015

No news yet. I just released Redux 1.0 a few weeks ago and has been busy with React Hot Loader 2 recently which needs a lot of work and has much more priority. When I'm idle, I'll look at this, but for now, feel free to release this separately if I'm being frustrating. Sorry!

@janusch

This comment has been minimized.

Show comment
Hide comment
@janusch

janusch Jan 20, 2016

Is this going to be merged? Would suit your, @gaearon , functional style much better than extending from PureComponent.

janusch commented Jan 20, 2016

Is this going to be merged? Would suit your, @gaearon , functional style much better than extending from PureComponent.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 13, 2016

Owner

Ugh. I didn’t handle this one well. I’m sorry! 😥

I decided to deprecate this project, as there are better maintained options now:
https://github.com/gaearon/react-pure-render#no-maintenance-intended

Thank you for sending the PR, and sorry for the trouble!

Owner

gaearon commented Mar 13, 2016

Ugh. I didn’t handle this one well. I’m sorry! 😥

I decided to deprecate this project, as there are better maintained options now:
https://github.com/gaearon/react-pure-render#no-maintenance-intended

Thank you for sending the PR, and sorry for the trouble!

@gaearon gaearon closed this Mar 13, 2016

@taion taion deleted the taion:decorator branch Mar 13, 2016

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Mar 13, 2016

Contributor

No worries.

Contributor

taion commented Mar 13, 2016

No worries.

@janusch

This comment has been minimized.

Show comment
Hide comment
@janusch

janusch Mar 15, 2016

Which alternatives are you thinking of? This one: https://github.com/acdlite/recompose ?

janusch commented Mar 15, 2016

Which alternatives are you thinking of? This one: https://github.com/acdlite/recompose ?

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