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

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

Open
towerofnix opened this issue Mar 8, 2023 · 5 comments
Assignees

Comments

@towerofnix
Copy link
Member

towerofnix commented Mar 8, 2023

(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
Copy link
Collaborator

This looks like a good change to me!

@PullJosh
Copy link
Collaborator

PullJosh commented Mar 9, 2023

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
Copy link
Collaborator

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
Copy link
Member Author

Thanks for the perspective both!

@adroitwhiz, I mentioned this subject in another issue: #169 (comment)

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 towerofnix changed the title Use Leopard's sprite constructor vars option instead of setting initial variables one at a time toLeopard: Use Leopard's sprite constructor vars option instead of setting initial variables one at a time Mar 10, 2023
@towerofnix towerofnix added the discussion Looking for feedback and input label Mar 10, 2023
@towerofnix towerofnix changed the title toLeopard: Use Leopard's sprite constructor vars option instead of setting initial variables one at a time toLeopard: Investigate how custom Sprite and Stage subclasses type the _vars property Mar 11, 2023
@towerofnix towerofnix removed the discussion Looking for feedback and input label Mar 11, 2023
@towerofnix
Copy link
Member Author

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!

@towerofnix towerofnix transferred this issue from leopard-js/sb-edit Mar 11, 2023
@towerofnix towerofnix changed the title toLeopard: Investigate how custom Sprite and Stage subclasses type the _vars property Investigate how custom Sprite and Stage subclasses type the _vars property Mar 11, 2023
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

No branches or pull requests

3 participants