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.

Sign up for GitHub

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

Jump to bottom

React.renderComponent() tries to update components without checking their constructor type #100

Closed
petehunt opened this issue Jun 17, 2013 · 2 comments
Closed

React.renderComponent() tries to update components without checking their constructor type #100

petehunt opened this issue Jun 17, 2013 · 2 comments
Assignees
@yungsters

Comments

@petehunt
Copy link
Contributor

See this jsfiddle: http://jsfiddle.net/j7skc/1/
Reported here: https://groups.google.com/forum/?fromgroups#!topic/reactjs/tCFACnPMZ6Q

Looks like we aren't checking the constructor type to figure out that we should unmount rather than receive new props. This may be fixed in master already (haven't checked); @yungsters or @jordow can you take a look (and add a test)?
@ghost ghost assigned yungsters Jun 17, 2013
@zpao
Copy link
Member

zpao commented Jun 17, 2013

I think this got fixed by 100af48, but haven't confirmed

@zpao
Copy link
Member

zpao commented Jun 17, 2013

And confirmed that it's working in master.

@zpao zpao closed this as completed Jun 17, 2013
@mflash123 mflash123 mentioned this issue Mar 18, 2019
Closed
@bvaughn bvaughn mentioned this issue Dec 17, 2019
Open
taneliang referenced this issue in MLH-Fellowship/react Aug 17, 2020
@taneliang
[Improve base View][2/n] Rationalize view layout system (#100)
6267aac 
Summary

The layout system was previously a bit of a mess. Only content views
implemented the `desiredSize` method, and thus composing other views in
`StaticLayoutView` was problematic as our layouters need `desiredSize`
as a size hint.

This (large) PR attempts to fix this by making some major changes to the
layout system:

1. Increase the usefulness of layouters, by defining many more layouters
   and allowing them to be composed. They are currently not used, but
   were tested to work before I realized I didn't need them in this PR.
2. Move layouters to the base `View` class, allowing all views to have
   some default layout behavior. The default layout behavior is still a
   noop.
3. Add a default `desiredSize` implementation to `View`. The default
   implementation simply lays out its subviews, then hugs them.
4. Update `VerticalScrollView` and `HorizontalPanAndZoomView` to work
   with the new `View`. As these classes are essentially a combination
   of a layouter + an interactor, we may be able to split them into
   injectable functions in the future.
5. Update `FlamechartView` to work with the new `View`. As this view is
   basically composed from a bunch of layouty views and content views
   (think a React component that uses a bunch of other React
   components), it can now take advantage of layouters on the base
   `View`.

Test Plan

* `yarn lint`
* `yarn flow`: no change in errors
* `yarn start`: appears to be equally performant
taneliang referenced this issue in MLH-Fellowship/react Aug 19, 2020
@taneliang
[Improve base View][2/n] Rationalize view layout system (#100)
32a33b0 
Summary

The layout system was previously a bit of a mess. Only content views
implemented the `desiredSize` method, and thus composing other views in
`StaticLayoutView` was problematic as our layouters need `desiredSize`
as a size hint.

This (large) PR attempts to fix this by making some major changes to the
layout system:

1. Increase the usefulness of layouters, by defining many more layouters
   and allowing them to be composed. They are currently not used, but
   were tested to work before I realized I didn't need them in this PR.
2. Move layouters to the base `View` class, allowing all views to have
   some default layout behavior. The default layout behavior is still a
   noop.
3. Add a default `desiredSize` implementation to `View`. The default
   implementation simply lays out its subviews, then hugs them.
4. Update `VerticalScrollView` and `HorizontalPanAndZoomView` to work
   with the new `View`. As these classes are essentially a combination
   of a layouter + an interactor, we may be able to split them into
   injectable functions in the future.
5. Update `FlamechartView` to work with the new `View`. As this view is
   basically composed from a bunch of layouty views and content views
   (think a React component that uses a bunch of other React
   components), it can now take advantage of layouters on the base
   `View`.

Test Plan

* `yarn lint`
* `yarn flow`: no change in errors
* `yarn start`: appears to be equally performant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yungsters yungsters

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@zpao @yungsters @petehunt