Recursion on classes when a method is used as a prop (ie onClick)

[titoBouzout]

I stumble upon on a recursion issue, The following:

class App extends Component {
  onClick() {}
  render() {
    return <Box onClick={this.onClick} />;
  }
}

compiles to

class App extends _mobxJsx.Component {
    onClick() {}
    render() {
        return (0, _mobxJsx.createComponent)(Box, {
            get onClick() {
                return this.onClick
            }
        })
    }
}

Demo:
https://codesandbox.io/s/mobx-counter-context-bug-forked-wp9m9?file=/index.js

[ryansolid]

I see. This is the only one of my libraries that supports classes and thus the only library that has to worry about this. This is actually pretty awkward. We'd need to alias the this and hoist it to the top level of the template. And in the case of nesting resolve it upwards.

This is result of a recent change to create the getters at compile time instead of runtime for less code and better performance. We were using arrow functions before.

This is going to take me sometime to figure out as the problem is in the compiler. I'd want to solve it in a generic way that would not affect usual output. So thank you for your patience this could take a couple weeks.

I'm debating just dropping support for class components in the meanwhile. I'd suggest using older versions but they had some other serious bugs as you know.

Or I guess you could alias this yourself for now. Like add const _self = this; at the top of the render function temporarily. And bind to _self.

EDIT: Classes are sort of awkward in general because of the property access I can't tell if it is observable at compile time. onClick shouldn't even be wrapped in the normal case. It's a small overhead but I can't avoid it.

[titoBouzout]

I understand, I prefer to work in classes because allows me to group code in somewhat logical ways that are related to the same thing, specially if you have many methods that are related but slightly different. its a lot easier than hooks to read it. Is just a preference I guess. Does Solid support classes? Take this very short example, in my opinion is a lot clear the class than the hook

class Item extends Component {
	@observable hidden = false
	@observable content = ''
	constructor() {
		event.on('content', this.setContent)
	}
	onMount() {
		console.log('hola')
	}
	onUnmount() {
		even.off('content')
	}
	setContent(content) {
		this.content = content
	}
	toggle() {
		this.hidden = !this.hidden
	}
	render() {
		return (
			<div>
				{!this.hidden && <div>{this.content}</div>}
				<div onClick={this.toggle}>toggle</div>
			</div>
		)
	}
}

function Item() {
	const [state, setState] = createState({ hidden: false, content: '' })

	event.on('content', content => {
		setState({ content: content })
	})

	Promise.resolve().then(() => {
		console.log('hola')
	})
	
	function toggle() {
		setState({ hidden: !state.hidden })
	}

	clean(() => {
		even.off('content')
	})
	return (
		<div>
			{!state.hidden && <div>{state.content}</div>}
			<div onClick={toggle}>toggle</div>
		</div>
	)
}

[ryansolid]

Yeah Solid does not support Classes and I don't intend to. I understand people do have preferences though. Mine is I've used Hook like syntax for basically a decade so all the code grouping stuff, and abstraction that React was pushing with the Hooks release was something I'd been promoting and subscribing to for years. In fact, missing that was part of my initial (mostly unfounded) dislike for React.

I did use classes with these sort of primitives but basically 90% of the code was in the constructor and all the class methods were event handlers that would need to be bound anyway. The reason being each component runs once and there is no centralized lifecycle. The class components I have here are more or less just call render function in the constructor. So while I could see an argument for the organization in general from a library interface design perspective, these libraries have no use for them. I know not always necessarily the case(and MobX actions are a reason to consider otherwise) but if you inline the simple event handlers classes in a library like this end up looking like:

class Item extends Component {
	@observable hidden = false
	@observable content = ''
	constructor() {
		event.on('content', (e) => this.content = content);
                cleanup(() => event.off('content'));
                setTimeout(() => console.log('hola'));
	}
	render() {
		return (
			<div>
				{!this.hidden && <div>{this.content}</div>}
				<div onClick={() => this.hidden = !this.hidden}>toggle</div>
			</div>
		)
	}
}

Compared to:

function Item() {
        const state = observable({
                 hidden: false,
                 content: ''
         });
	event.on('content', (e) => state.content = content);
        cleanup(() => event.off('content'));
        setTimeout(() => console.log('hola'));
        return (
		<div>
			{!state.hidden && <div>{state.content}</div>}
			<div onClick={() => state.hidden = !state.hidden}>toggle</div>
		</div>
	);
}

It's basically identical in the simple case. Even if you don't inline the event handlers you just make a block where you put them. Where things differ is how to group things. Lifecycles basically enforce grouping that way:

class {
    dataA = ""
    dataB = ""
    dataC = ""
    lifecycle1() {
        fn(dataA);
        fn(dataC);
    }
    lifecycle2() {
        fn(dataB);
        fn(dataC);
    }
    lifecycle3() {
        fn(dataA);
    }
}

Whereas you could group it that way but can also group it this way:

fn() {
    // A
    dataA = ""
    primitive1(fn(dataA));
    primitive3(fn(dataA));

    // B
    dataB = ""
    primitive2(fn(dataB));

    // C
    dataC = ""
    primitive2(fn(dataC));
    primitive3(fn(dataC));
}

Preference for sure but when the updates themselves can be more granular this makes a lot of sense. And I suspose my comments in the last example are actually what makes it clearer instead of having a programming construct to enforce it. But I suppose I could if I mean each of A/B/C into their own hook/primitive.

EDIT: To my point about onClick. I can't tell that this.onClick is reactive or not where as I know onClick is not as there is no property access or function call. It's a small thing but it is a good optimization that I can make. You of course could do onClick = this.onClick in this case too. But for some reason just dealing with variables in scope clicks for me.

[titoBouzout]

Thanks a lot Ryan. I see your point and I think you are right the classes way could lead to a false notion of organization. I will maybe give it a try to the hooks way . I do not expect an update to the classes implementation. Thanks again!

[ryansolid]

To be fair this was all true before I added the classes here. It was mostly for decorator syntax support. MobX 6 gets away from it and you can leverage makeObservable on objects.

But I still dislike that this broken. I know people are used to MobX like this, it's just super unfortunate because I have a few other priorities before I get back there. There are other scenarios this breaks. I know a few libraries that use Solid this way with Custom Elements that would break, so I for sure have to fix this. I'm just in the middle of a large refactor.

[ryansolid]

Ok this is next on my list assuming I can do it easily enough. I want to get this in for my next minor release of my libraries but I don't want it to delay the next Solid release.

[ryansolid]

Ok happy to say fixed in babel-plugin-jsx-dom-expressions v0.20.8. Let me know if you have any more issues.