tmountain / pico-8-typescript

Create PICO-8 games in TypeScript
MIT License
38 stars 1 forks source link

Edit jspicl-cli to allow the usage of classes #2

Closed QuentinWidlocher closed 2 years ago

QuentinWidlocher commented 2 years ago

If you create a basic class with a method in Typescript like this :

class MyClass {
  public myMethod() {
    return "Hello World";
  }
}

let instance = new MyClass()

function _init() {
  instance.myMethod();
}

and try to run pico-8-ts, you'll get a nasty error telling you :

attempt to index local 'myclass' (a function value)

If we look at compiled.js we can see what causes this :

var MyClass = /** @class */ (function () {
    function MyClass() {
    }
    MyClass.prototype.myMethod = function () {
        return "Hello World";
    };
    return MyClass;
}());
var instance = new MyClass();
function _init() {
    instance.myMethod();
}

The javascript transpiled by tsc targets ES5, so it doesn't use class and Pico 8 Lua doesn't support prototypes.

Except that if you change the tsconfig.json to target ES6, even if you get this correct javascript, the problem persists.

class MyClass {
    myMethod() {
        return "Hello World";
    }
}
let instance = new MyClass();
function _init() {
    instance.myMethod();
}

Pico 8 still cries about protoypes except we don't have any prototypes in our generated output anymore.

It's because of jspicl-cli.
In /node_modules/jspicl-cli/bin/cli at line 100 we can see that it uses Bublé to compile js into < ES2015.
By default, Bublé strips away classes and use protoypes instead, and that's what transforms our class.

I removed Bublé and got an error, so instead I've just added the option to skip over classes and leave them alone :

function getInputOptions({ input, output, ...jspiclOptions }) {
  return {
    input,
    plugins: [
      includePaths({
        paths: [path.resolve(input)]
      }),
+      buble({
+        transforms: {
+          classes: false,
+        }
+      }),
      {
        renderChunk: source => source.replace(/\/\/ <!-- DEBUG[^//]*\/\/\s-->/g, "")
      },
      jspiclPlugin(jspiclOptions)
    ]
  };
}

Now it works fine. Should we create another patch over jspicl-cli ? Or do we make a fork so we can change #1 , this and maybe even replace the jspicl dependency to @jspicl/jspicl so we can migrate to version 3.0.0 and enable ESM ?

tmountain commented 2 years ago

I think a fork is the best option because the original project was abandoned. Do you want to create it?

tmountain commented 2 years ago

Also, if a fork is created, a lot of the npm packages need to be audited...

QuentinWidlocher commented 2 years ago

I can create it, but I'm afraid of breaking the compatibility with older versions of node, since you're telling me you don't have any problems with node 12.

tmountain commented 2 years ago

We can just update the README to make the latest Node a dependency. It's a new project, so I'm not too worried about it.

QuentinWidlocher commented 2 years ago

I've created a fork for jspicl-cli. I've tested it locally with pico-8-typescript and it work as expected.

I'm ready to publish a package on npm and create a PR here, but I'm not sure how I should name the npm package ?

tmountain commented 2 years ago

I tested your code with Node 12 and 18, and it works with both. Regarding publishing the package, can't you just leave the name the same, but scope it to your username? I.e., "name": "@quentinwidlocher/jspicl-cli"?

tmountain commented 2 years ago

Marking this as resolved thanks to your PR.