leopard-js / leopard

Library for making Scratch-like projects with JavaScript.
https://leopardjs.com
MIT License
136 stars 26 forks source link

Deprecate `sprite.vars.*` in favor of using class properties directly? #196

Open PullJosh opened 1 month ago

PullJosh commented 1 month ago

Right now, Leopard targets (sprites and the stage) have a .vars object. Projects converted via sb-edit have all the Scratch variables converted to key/value pairs on the vars object.

When I edit Leopard projects manually, I am constantly annoyed by this and choose to just assign things directly to the sprite itself instead, like so:

// Current
this.vars.n = 5;

// What I prefer
this.n = 5;

This gives a far more "normal" JavaScript development experience, which I think is better for Leopard users who are learning to write JS code.

The only argument I can think of in favor of vars is that it provides a namespace so that you can, for example, have this.vars.x be a variable and this.x be the sprite's x-position. In my opinion, it would be better to stop using this namespace and instead have sb-edit convert to this.var_x and this.x or similar.

towerofnix commented 1 month ago

Totally love it. Epic basketball score, lol.

this.vars is a nasty evil on Leopard as an educational tool and it just doesn't need to be there. It literally doesn't need to be there! You write code that uses this.n = 5 and it works! Gosh darn it!

I do think there are going to be awkwardnesses with project conversion that we... can't do a lot to avoid very prettily. I mean, the thing is that Leopard uses this.x to represent the sprite's position, whereas in Scratch that block is called "x position". So presumably, most Scratchers don't create their own variable named "x position", because that's awkward and hard to tell apart... they just make one named "x"!

I wonder if we can avoid using names like this._x. It's just awkward, and one (very small) character isn't enough to quickly tell this.x from this._x. Can we do this.xVar instead? It's not pretty, but it avoids the confusion, right?

Of course if sb-edit had some rather terrifying static analysis (https://github.com/leopard-js/sb-edit/issues/98#issuecomment-1858618853) then we could just identify if a variable is only used within the function it converts into, and then just define it as a lexically bound variable with let. Buuuut it's okay to go without that and succumb to this._x or this.xVar or something similar, meanwhile, and when the case is necessary anyway.

PullJosh commented 1 month ago

Love it. We can bikeshed the naming collision situation as much as we want. I'm happy with this._x because it feels like it nicely denotes the "private" x that this sprite has as opposed to its public x coordinate. Or this.xVar or this.varX help users clearly understand which is the variable and which is the position.

And yes, terrifying static analysis is absolutely a good idea, once we build up the courage. Anything that can be a local variable should be a local variable. (Maybe at some point in the future we'll revise this statement, but I think it's at least mostly correct.)

towerofnix commented 1 month ago

I like the "private" thought, but I want to mention that it sort of conflicts with the this.sprites.MySprite._x syntax for the "(attribute/variable) of (sprite)" Sensing block. Maybe not fundamentally, but I do think there's a bit of an inconsistency if we don't name all for-this-sprite-only variables with an underscore. And that's just not JavaScript-y—JS has private class fields (i.e. private identifiers) to that end, and a sprite might expose that with get xPosed() { return this.#x }... (Bad pun...)

Incidentally, this._x is already a taken name by Leopard internals, but that's neither here nor there.

PullJosh commented 1 month ago

Alright, I think that's a good enough reason not to choose the underscore syntax.

Personally, between this.varX or this.xVar, I vote for varX. No clear reason why. I just find it easier to read in my brain. It helps me narrow in that first I'm thinking about a variable, and then that I'm thinking about the x variable specifically (kind of like how a YYYY/MM/DD format progressively narrows in a date). Although, to be honest, you could make the exact opposite argument as well, so it's really just my unfounded personal preference.

towerofnix commented 1 month ago

OK, so we just wrote up https://github.com/leopard-js/sb-edit/issues/138#issuecomment-2132171014 and found that _x and _y are not in the list of names Leopard actually internally uses! So... clearly the TypeScript compiler and/or a minifier is optimizing those private names out. We would not be conflicting with them by using _x, _y etc. But yeah, it's still not very idiomatic, especially if other sprites read it.

Anytime verbiage could go one way or another and the world wouldn't end either way, we tend to not worry too badly about the outcome, LOL. Between this.varX and this.xVar we're happy either way.

Bikeshedding is fun but also boring, so if you're conflicted and would like an outside decision to help you move on (we've been there lol), we vote varX.