leopard-js / leopard

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

Investigate how custom Sprite and Stage subclasses type the _vars property #174

Open towerofnix opened 1 year ago

towerofnix commented 1 year ago

(Original title, created in sb-edit repository: Use Leopard's sprite constructor vars option instead of setting initial variables one at a time)

Current implementation for initial variable values:

`
${[...target.variables, ...target.lists]
  .map(
    variable =>
      `this.vars.${variableNameMap[variable.id]} = ${toOptimalJavascriptRepresentation(variable.value)};`
  )
  .join("\n")}
`

Proposed:

const variablesAndLists = [...target.variables, ...target.lists];
const initialVariables =
  (variablesAndLists.length
    ? "{\n" + variablesAndLists
        .map(variable => `${variableNameMap[variable.id]: ${toOptimalJavascriptRepresentation(variable.value)},`)
        .join("\n") + "\n}"
    : '');

`
constructor(options) {
  ${initialVariables ? `super(options, ${initialVariables});` : `super(options);`}
`

Example difference in output - before:

export default class Stage extends StageBase {
  constructor(...args) {
    super(...args);
    ...

    this.vars.scrollX = -18.094999999999995;
    this.vars.p1 = "Green";
    this.vars.p2 = "Red";
    this.vars.map = "Grasslands";
    ...
  }
}

After:

export default class Stage extends StageBase {
  constructor(options) {
    super(options, {
      scrollX: -18.094999999999995,
      p1: "Green",
      p2: "Red",
      map: "Grasslands"
    });
    ...
  }
}

The first argument options is still needed for constructing each sprite class with initial position, costume, etc in index.js (of the generated output).

The second argument vars, despite being present on Leopard's exposed Sprite and Stage constructors, so far goes completely unused, even within Leopard (which bypasses a given sprite's constructor when cloning, so this change doesn't affect that).

/cc @PullJosh @adroitwhiz This is a significant change in Leopard code serialization, so I'd like to get both yours' thoughts or approval before going ahead with it.

adroitwhiz commented 1 year ago

This looks like a good change to me!

PullJosh commented 1 year ago

Only question: Do we think it's more educational, for people who are just learning JavaScript, to see a more "normal" way of assigning a value to a variable? It's very common in a constructor to have code that looks like

constructor(a, b) {
  this.a = a;
  this.b = b;
  this.c = Math.sqrt(a**2 + b**2);
}

The current format matches that pattern more closely.

Additionally, the assignments in the constructor can be a hint to people as to how they should assign values to variables later on in their code. It's nice to be able to copy + paste directly into other parts of the code, which can't happen with the passed object version.

If we were expecting Leopard to be used by JS experts, I think the slightly more terse notation would be nice. But in this case, are we actually doing beginners a favor by abstracting away the assignment/introducing an additional way of doing things, or are we obfuscating what's going on?

adroitwhiz commented 1 year ago

My main concern is with TypeScript. If we're expecting everyone to write JS since that's what projects are converted to, the existing way works perfectly fine. If anyone wants to write a Leopard project in TypeScript, they'll run into issues because vars is typed as an empty object and they can't access any properties on it.

It may also be possible for the child sprite class to just override the type of its own _vars? I'll have to look more into that.

towerofnix commented 1 year ago

Thanks for the perspective both!

@adroitwhiz, I mentioned this subject in another issue: https://github.com/leopard-js/leopard/issues/169#issuecomment-1462455070

I think we need to identify how/when TypeScript will be exposed to the end-user — in terms of both working off of codesandbox projects generated from the online site and working locally, where a tsc build loop may be more accessible — and consider that when deciding how much TypeScript should shape Leopard's (user-facing) code style.

@PullJosh, I agree overall with the benefits to the current syntax; just as an addendum, I think the ways Leopard familiarizes users with real-world JS by reflecting common JS patterns is something to explore more in the future too — just for example, right now we are using triggers for "when green flag clicked" and "when I start as a clone" that are sometimes very close to something you'd usually see in a constructor, and if we can be smart (and compatible), that's one thing we could translate in a closer-to-"normal JS" kind of way. There's probably a lot more to be explored in the general area!

towerofnix commented 1 year ago

I think we're going to stick with the existing syntax for now, but feel free to resume the conversation on that or similar subjects at any time. @adroitwhiz, I'm assigning this to you for inspecting how the _vars property is handled in TypeScript (whenever), since it's a todo you mentioned planning to investigate anyway!