Skip to content

Conversation

@rvdende
Copy link

@rvdende rvdende commented Sep 9, 2019

Just a simple quick fix to get rid of the react 17.x messages. If you guys think this is not the best way to resolve it and would rather go the componentDidUpdate route feel free to delete this PR.

@STRML
Copy link
Collaborator

STRML commented Sep 9, 2019

Would be good to get this to static getDerivedStateFromProps(props, state).

@rvdende
Copy link
Author

rvdende commented Sep 10, 2019

@STRML cool I've updated the PR to use static getDerivedStateFromProps(props, state). I've not managed to get yarn to stop throwing me with errors though. Perhaps its time to move this package over to typescript? It is strange to have types in .js files for me?

@rvdende rvdende changed the title Renamed to UNSAFE_componentWillReceiveProps new react getDerivedStateFromProps Sep 15, 2019
@daynin
Copy link

daynin commented Sep 18, 2019

It is strange to have types in .js files for me

this package uses flowtype (not typescript), that's why we use .js extension instead of .ts

@rvdende
Copy link
Author

rvdende commented Sep 18, 2019

ah I see, thanks for the comment @daynin

});
}
if (!state.resizing &&
(props.width !== props.width || props.height !== props.height)) {
Copy link

Choose a reason for hiding this comment

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

It won't work

props.width !== props.width // false (always, except cases with NaN)

you should use componentDidMount here

height: nextProps.height
});
static getDerivedStateFromProps(props: ResizableProps, state:Object) {
if (props.width !== props.width || props.height !== props.height) {
Copy link

Choose a reason for hiding this comment

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

the same case as above

@petejodo
Copy link

The react-draggable dependency had a very similar PR and was published as a major version, could this bump that dependency so that it removes all usage of the old react APIs. As of right now, including this change will still trigger warnings in v16.9 due to the react-draggable dependency

});
}
if (!state.resizing &&
(props.width !== props.width || props.height !== props.height)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comparison doesn't make sense. You don't have access to the previous props. See react-grid-layout/react-draggable@fea778c

};

componentWillReceiveProps(nextProps: Object) {
static getDerivedStateFromProps(props: Object, state:Object) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use Props & State types

STRML added a commit that referenced this pull request Oct 22, 2019
This must have been a vestigal bit of state from before the Resizable/ResizableBox
refactor years ago. It is not actually needed and with the React 16.9 refactor
to remove CWRP, it became obvious it was actually not useful.

Fixes #99
Supersedes #100
Partially fixes #107, #110
STRML added a commit that referenced this pull request Oct 22, 2019
This must have been a vestigal bit of state from before the Resizable/ResizableBox
refactor years ago. It is not actually needed and with the React 16.9 refactor
to remove CWRP, it became obvious it was actually not useful.

Fixes #99
Supersedes #100
Partially fixes #107, #110
@STRML
Copy link
Collaborator

STRML commented Oct 22, 2019

Closing in favor of #112

@STRML STRML closed this Oct 22, 2019
STRML added a commit that referenced this pull request Oct 24, 2019
* refactor(Resizable): remove draggable state

This must have been a vestigal bit of state from before the Resizable/ResizableBox
refactor years ago. It is not actually needed and with the React 16.9 refactor
to remove CWRP, it became obvious it was actually not useful.

Fixes #99
Supersedes #100
Partially fixes #107, #110

* fix(ResizableBox): Fix 16.9 CWRP deprecation
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