Unable to use ROT 2.0.2

mingos777 commented 5 years ago

As of 2.0.2, ROT is compiled to es2017. This target makes it completely unusable in a Node.js (v10.14.1) application (even with --experimental-modules flag, as it requires .mjs file extensions).

I realise that es2017 features are actually in use (tsc complains about padStart() method use when compiling to es6), but these are rather easy to polyfill.

ondras commented 5 years ago

For node, please use the dist/rot.js. This is the file used in the node.js example. It is also referenced package.json's main field: https://github.com/ondras/rot.js/blob/master/package.json#L30

mingos777 commented 5 years ago

Hm. This should work then. Yet it completely borks my TypeScript compiler.

The code I used was something along these lines:

import * as ROT from "rot-js";

// ...irrelevant

const preciseShadowcasting: ROT.FOV.PreciseShadowcasting = new ROT.FOV.PreciseShadowcasting(
    (x: number, y: number) => map.isTransparent(x, y)
);

The compiler complains about the namespace FOV not existing.

ondras commented 5 years ago

The compiler complains about the namespace FOV not existing.

Can you find out whether this resolves to src/, lib/ or dist/? The repo happens to contain three distinct versions of the codebase...

rot.js itself is compiled with TS (and tsc understands es2015 modules), so for a TS-based project, I would try to use the original non-transpiled non-bundled version from src/. Unfortunately, I am not yet well versed in TypeScript, so my ability to help here is quite limited. I hence summon @lostfictions, our TS guru and author of the initial TS implementation!

mingos777 commented 5 years ago

I'll try to determine that after work today.

Using the contents of src/ isn't exactly a possibility when rot.js is installed via npm. It will resolve to what package.json points to, so src/ is pretty much inaccessible there. BTW, the TS source files should probably be added to .npmignore.

For the FOV tests that I'm running, I temporarily solved the issue by saving rot.js (dist version) in a separate repository with hand-rolled TypeScript typings for the FOV module. This is suboptimal since whenever a new version is released, I will either need to update this manually or rely on an outdated version to do the testing, but for now, this was the only thing that worked for me.

mingos777 commented 5 years ago

OK, I fiddled around a bit with the project and it seems I can use it after all, albeit not without adjustments.

Here's my test code:

import * as ROT from "rot-js";
import PreciseShadowcasting from "rot-js/lib/fov/precise-shadowcasting";

const map: Array<string> = [
    "#########",
    "#.......#",
    "#.....###",
    "#.......#",
    "#...@...#",
    "#####...#",
    "#.......#",
    "#...#...#",
    "#########"
];
const visibility: Array<string> = [
    "         ",
    "         ",
    "         ",
    "         ",
    "         ",
    "         ",
    "         ",
    "         ",
    "         "
];

const fov: PreciseShadowcasting = new ROT.FOV.PreciseShadowcasting(
    (x, y) => map[y].charAt(x) !== "#"
);

fov.compute(
    4,
    4,
    Infinity,
    (x, y) => visibility[y] = visibility[y].substr(0, x) + map[y].charAt(x) + visibility[y].substr(x + 1)
);

console.log(visibility.join("\n"));

Initial attempt to compile it (again, using rot.js 2.0.2 from npm) yields an error:

mingos@Yodeling-Elk:~/dev/rot-js-test$ tsc
node_modules/rot-js/lib/lighting.d.ts:1:17 - error TS2307: Cannot find module 'fov/fov.js'.

1 import FOV from "fov/fov.js";
                  ~~~~~~~~~~~~

Found 1 error.

This is the issue I submitted a PR for. After fixing the import in lib/lighting.d.ts, the compilation is successful. I added a console.log("DIST") line at the very beginning of the dist version, and sure enough, when I run the test program, I get this:

mingos@Yodeling-Elk:~/dev/rot-js-test$ node src/index.js
DIST!
########
#......
#.....###
#.......#
#...@...#
#####...#
     ...#
      ..#
      ###

So to answer your question: TypeScript uses the dist version of the codebase.

Also, it appears that I can in fact use rot.js using TypeScript, but instead of accessing types through the imported ROT object, I need to specifically import the classes from a given file. It's doable provided the editor has code completion and you know exactly what you're looking for. Otherwise, I think the TS exports could use some reorganisation in rot.js code.

I can offer some assistance with this if needed.

lostfictions commented 5 years ago

Yeah, the organization of things in the project is definitely not ideal IMO. I haven't really dug into the build process @ondras ended up with, but I had made some recommendations that I think kind of ultimately got ignored :(

(Not strictly related to the problem at hand, but one thing that jumps out at me immediately looking at the Makefile is the use the use of inotifywait over the entire source directory to trigger rebuilds instead of just... tsc's watch mode, which does incremental compilation and would definitely be faster/simpler: https://github.com/ondras/rot.js/blob/ab046a2425dbde7ab6262de584be348fae8e6a5d/Makefile#L41-L42

FWIW I think I mentioned it before but I feel like using stuff like Make and idiosyncratic build chains renders the project a lot more inaccessible to potential contributors and folks who want to help, especially if they don't or can't have a traditional Unix toolchain installed.)

For my part I'm not sure how the Closure compiler build artifacts interact with type definitions, or whether there's a sensible way they can coexist. ...I also definitely said this, but I think only offering the two extremes of "ES5 global namespace dump" and "full ES modules transpile-it-yourself" is not sufficient for most users, which is maybe the problem we're seeing now (in tandem with typedef generation problems).

I guess if I were to recommend again how I'd do it, I'd pass the original TypeScript sources through Babel, which will happily parse TS code and produce ready-to-use downlevelled files. You could use the existing, widely-used Babel Rollup plugin to build (and optionally minify) for both the browser and Node this way. Alternately, if you want to keep using Closure, there's also a Rollup plugin for it that might be simpler for producing artifacts that are more ready-to-use (I'm sure you could use it in tandem with the TypeScript plugin).

mingos777 commented 5 years ago

@ondras I guess this remark really goes to you. Is a new build process acceptable? If so, I'm more than happy to help when I find some spare time.

mingos777 commented 5 years ago

@lostfictions My guess would be that type declarations and closure compiler have nothing to do with each other. Type declarations are produced by tsc, tsconfig.json is configured to produce them:

https://github.com/ondras/rot.js/blob/ab046a2425dbde7ab6262de584be348fae8e6a5d/tsconfig.json#L7

It takes the TypeScript sources and spits out the type declarations based on the types and the code structure. Closure compiler takes JavaScript as input and outputs JavaScript as well, just minified and, as Google brags, "faster, with removed dead code" and whatnot.

Personally, I don't see a good reason to use a Java tool in the first place, taking into consideration there are Node.js projects that do the same (uglify, anyone?). If I may drop a recommendation, I'd much prefer a toolchain consisting of Node-only utilities. That would make the build process straightforward and accessible to anyone. I'm not well versed in webpack as I don't work with client-side JS, but I guess that would be the tool of choice to produce the output code. Or Gulp + TypeScript + UglifyJS + Browserify (which is something I've set up in the past).

Also, are the ES2017 modules used for anything apart from feeding Babel with them? Because as far as I can tell, TypeScript will be more than happy to produce code that will be usable in Node.js directly. I picture it this way:

TypeScript takes src/**/*.ts and spits out lib/**/*js + typings with target = "es6" and module = "commonjs". Boom, rot.js is perfectly usable in Node.js in this form.
Browserify takes lib/**/*.js and spits out dist/rot.js - merged in a single file and with the commonjs modules translated to whatever the browser can chew.
UglifyJS takes dist/rot.js and spits out dist/rot.min.js

No other tools are really needed. I don't really see the point in using es2017 modules since they are not usable by anything. Nothing really supports them and when rot.js is downloaded via npm, only what the package.json points to is exposed anyway, so the JS contained in the lib/ directory is essentially dead.

ondras commented 5 years ago

This issue has taken quite a twist! So many topics are making it somewhat hard to follow. I will try to address individual points, but let's start with some general observations first.

1) rot.js is primarily meant for client-side development. The node support is something naturally available due to the language used, but my wild guess is that 80% of rot.js use cases are for browser-based games.

2) The code produced by Closure Compiler is aimed at client-side usage where

2a) code size matters,
2b) folks might not be used to complex build setups.

It is a pre-built version wher you just want to download and include in your Githubissues.

Githubissues is a development platform for aggregating issues.

ondras / rot.js

Unable to use ROT 2.0.2 #148