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

[JSX] Unconfigured props will not be overwritten if a new undefined value is not passed in #374

Closed
bryceosterhaus opened this issue May 2, 2018 · 9 comments

Comments

@bryceosterhaus
Copy link
Member

In the example below, the GrandChild component will not render its default prop of 'Default Two after clicking the button. I believe this happens because metal.js calls addUnconfiguredProps but when that prop is not passed in anymore, it re-uses the old value because the prop is not explicitly passed in by its key anymore.

For example, if both foo and bar are unconfigured props and we pass {foo: 1, bar: 2} followed by passing in {foo: 2}. The component will continue to use bar: 2 because it doesn't know to remove that prop.

Check out this for a working example

import Component from 'metal-jsx';

class GrandChild extends Component {
	render() {
		return (
			<div>
				{this.props.one}
				<br />
				{this.props.two}
			</div>
		);
	}
}

GrandChild.PROPS = {
	one: {
		value: 'Default One'
	},
	two: {
		value: 'Default Two'
	}
};

class ChildComponent extends Component {
	render() {
		return (
			<div>
				<GrandChild {...this.otherProps()} />
			</div>
		);
	}
}

class Parent extends Component {
	handleClick() {
		this.state.active = !this.state.active;
	}

	render() {
		const props = this.state.active
			? {one: 'foo'}
			: {one: 'bar', two: 'baz'};
		return (
			<div>
				<button onClick={this.handleClick.bind(this)}>click</button>

				<br />

				<ChildComponent {...props} />
			</div>
		);
	}
}

Parent.STATE = {
	active: {
		value: true
	}
};

cc @mthadley

@mthadley
Copy link

mthadley commented May 2, 2018

I think conceptually, this is also a bug, because a JSX component's this.props should always reflect what was passed to the component. In the above example, where active is true, there is no prop now called two, so GrandChild shouldn't see the value 'baz'.

@travisrcory
Copy link

👎 Rob

@bryceosterhaus bryceosterhaus removed their assignment May 9, 2018
@eduardolundgren
Copy link
Contributor

cc @jbalsas

@jbalsas
Copy link
Contributor

jbalsas commented May 11, 2018

Hey guys, I'll take a look at this first thing next week!

This would be a breaking change... probably no-one is relying on this specific behaviour as it is, but I'll double check existing usages out there. I agree with the expected outcome.

@diegonvs
Copy link
Contributor

@brunobasto @interaminense

@diegonvs
Copy link
Contributor

diegonvs commented May 15, 2018

I think this would be a breaking change.

  • Are you guys trying to turn the props of a component be reset after a change in an unconfigured prop?
  • Props need be a long or short term state, considering this, if you change a value this will be undefined?

Let me know if I'm wrong. I'll search some use cases using React's definition of props, that we have inspired to build metal props idea.

@diegonvs
Copy link
Contributor

There is an issue, React's props have the same behavior as @bryceosterhaus explained.

@diegonvs
Copy link
Contributor

I'm taking this issue right now

@yuchi
Copy link
Contributor

yuchi commented May 16, 2018

It’s Historically Accurate Note Time™!

I'll search some use cases using React's definition of props, that we have inspired to build metal props idea.

Actually the State concept in Metal has been inherited from YUI Attribute infrastructure and then retrofitted into props for JSX components.

diegonvs added a commit to diegonvs/metal.js that referenced this issue May 16, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue May 16, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue May 16, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue May 16, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue May 16, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 18, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
diegonvs added a commit to diegonvs/metal.js that referenced this issue Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants