ondras / rot.js

ROguelike Toolkit in JavaScript. Cool dungeon-related stuff, interactive manual, documentation, tests!
https://ondras.github.io/rot.js/hp/
BSD 3-Clause "New" or "Revised" License
2.33k stars 254 forks source link

[discussion] Modernizing rot.js #132

Open lostfictions opened 6 years ago

lostfictions commented 6 years ago

Hey! I wanted to open a conversation about modernizing the rot.js codebase to make it a bit more maintainable, and potentially more welcoming to contributors too.

From reading the code over I think it's really well-engineered and very readable (albeit a bit sparsely commented in some places) but I think the way it's organized is a bit idiosyncratic for JS nowadays. I guess that's just an inevitable consequence of it being an old enough library to predate a lot of modern conventions, but to me it looks like a lot of the forks over the past year have had to do with making rot.js easier to use with modern build toolchains, more modular or compatible, etc.

I've gotten most of the way towards a modernized version of the library at my fork over here. I've opted to use TypeScript: I know it sort of goes against rot.js's "vanilla js" approach, but making use of ES modules right now inevitably means some kind of build step or preprocessing will need to be introduced (and besides, the current approach of building the web and Node versions through file concatenation, make and/or cake, etc. is tantamount to a build system anyway -- just a very ad-hoc one.) TypeScript also maps very naturally to most of the conventions used in rot.js -- abstract classes and interfaces are a pretty perfect fit throughout. Besides the internal benefits for maintainability and correctness, it also makes the library much easier to use for end users, since they'll get the benefit of high-quality autocomplete, errors, etc.

Since the library is so solidly engineered, a lot of the conversion thus far has been pretty mechanical -- most of the care has gone into eg. switching to block binding and moving declarations to inner scopes or closer to their usage site, figuring out object shapes and class fields and making them explicit, reifying implicit patterns, etc. The conversion isn't quite done yet, but it's going pretty smoothly so I thought it might be worth mentioning as a point of discussion.

Some other points of discussion:

Sorry if this is a long post! Again, I think the library is great as it is and only needs a bit of work to make it much more accessible.

ondras commented 6 years ago

Hey! I wanted to open a conversation about modernizing the rot.js codebase to make it a bit more maintainable, and potentially more welcoming to contributors too.

I agree with the general attitude here. Rot.js was written in pre-ES6 era and updating the code makes sense. (But takes time I am somewhat lacking now.)

If I was to update rot.js, I would definitely:

Further improvements (introducing let/const, destructuring, default args, async/await, ...) can be done incrementally.

I've gotten most of the way towards a modernized version of the library at my fork over here. I've opted to use TypeScript: I know it sort of goes against rot.js's "vanilla js" approach, but making use of ES modules right now inevitably means some kind of build step or preprocessing will need to be introduced

I am, unfortunately, not really familiar with TypeScript (I know what it is and what it does, but zero experience there). I would prefer sticking with Vanilla to ease code readability for newcomers (modernizing the codebase is done for them, right?).

Build step/preprocessing is present, albeit very dumb (concatenating stuff, as OP mentioned). Converting to ES6 modules of any kind does not modify the workflow.

I don't know if it makes too much sense to continue publishing a Bower package.

No, definitely not.

By a similar token, I don't think it's necessary to keep a separate subtree of Node overrides/shims -- I think any version of rot.js published to npm should be able to figure out whether it's running under Node or in a browser and offer the appropriate modules without any extra configuration.

(It's of course totally possible and desirable to keep offering a browser-ready, packaged rot.js and rot.min.js that don't require using the Node ecosystem, too.)

Browser-ready is mandatory. So this still means some code-splitting/alternative build paths. But perhaps the current node-related subtree can be reduced/simplified.

I haven't read over the tests extensively yet so I don't totally understand the overlap between the tests in node/test/ and the tests in test/.

I can speak only for test/ -- these are the original Jasmine-based tests that are meant to be run inside a browser. These cli-less tests are not very popular these days, but unfortunately they cover a reasonably large chunk of rot.js's functionality.

The Rhino-related stuff was an attempt to run said tests using a CLI.

Hopefully this isn't controversial, but I think the prototype extensions to array, number, string, etc. should be strictly opt-in.

I am personally still very OK with these modifications, because there was no single case of a naming collision that presented a real issue. All older reported issues turned out either bogus or as a problem of the conflicting party. But I seem to be in minority here :)

Having these opt-in is, unfortunately, backwards incompatible change. But if the user base does not want them, well, what can I do.

blinkdog commented 6 years ago

I haven't read over the tests extensively yet so I don't totally understand the overlap between the tests in node/test/ and the tests in test/. Intuitively the former would check the functionality of the Node-specific stuff, but it looks like they both seem to test common functionality, so it might be worthwhile to consolidate them to a common set of tests. I've experimentally passed the contents of node/test/ through the excellent decaffeinate tool in my fork as a first step towards consolidation.

The test/ and node/test/ overlap is due to how things evolved.

At first, rot.js was a pure browser library with no presence on Node. I forked it, made a shim for the browser objects, and created the "term" layout type for terminals. I pushed my code up to GitHub in case anybody wanted it, thinking nothing would probably ever come of it.

@statico liked the work and suggested an official Node release for the rot.js project. https://github.com/ondras/rot.js/pull/53

I was happy to help make that happen: https://github.com/ondras/rot.js/pull/54

After that, I decided to write a full coverage test suite for the entire library. https://github.com/ondras/rot.js/pull/65

The tests ended up under node/test/ because of a few reasons. I had a working build/test/coverage pattern in other projects, so bringing that pattern over to this project was easiest for me. My tests were in CoffeeScript and intended to be run by Node, so I thought it better to keep them tucked away in the Node fork, as opposed to the browser mainline. @ondras and I discussed consolidation with the Jasmine tests, and I thought it was a good idea. I never did get back around to doing it however.

I was very happy with the results of this coverage suite. We killed a ton of tiny bugs. https://github.com/ondras/rot.js/issues/61

Anyway, hopefully that clears up the test overlap thing. If you have any questions and/or would like some help, just let me know.

seanohue commented 6 years ago

It might be cool to have the library itself be in vanilla ES6, but to include TypeScript definition files for folks who want to write their game in TypeScript, or simply for the Intellisense support it offers.

lostfictions commented 6 years ago

Thanks for the detailed responses @ondras @blinkdog !

@seanohue I'm not a huge fan of maintaining typedefs out-of-band (that is, manually written as a separate definitions file) for a few reasons:

For what it's worth, my perspective on TS is that it's pretty similar to vanilla JS with Closure/JSDoc annotations, except with vastly better tooling that gives you immediate in-editor feedback and makes it nicer to write and consume code. The TS devs are pretty conservative about adding new language features; they'll basically only consider stuff that's at minimum a Stage 3 TC39 proposal (so as to not deviate too far from JS proper). It's a pretty big philosophical difference from most compile-to-JS languages (or even CoffeeScript, for all it claimed to be "just JavaScript.")

The upshot is that you can run a compiler pass that does full Babel-like transpilation to ES5/ES3 if you want, or you can just run a simple pass that strips type annotations and gives you code that's in modern JS but is otherwise more-or-less identical to what you wrote.

Anyway, comments welcome on my WIP fork! I've been trying to modernize everything but hew pretty closely to the original code, with a few exceptions.

lostfictions commented 6 years ago

Also, as @ondras noted, simply removing the prototype extensions is indeed a backwards-incompatible change. With my fork I haven't been taking any particular care to maintain API compatibility -- while I didn't want to make any drastic changes, I thought it might be a "clean break" to change some of the API surface in a few places. It could potentially be published as a new semver-major version and kept in a different branch of the repo.

If maintaining compatibility and sticking to vanilla ES5/6 is important to folks, I'm happy to maintain my fork as its own separate thing if that makes the most sense, and maybe in that case I'd make some more opinionated changes (and maybe contribute some of it back to this repo as vanilla JS where it makes sense?) Or anything in between -- it's probably clear that I'm a pretty big fan of TS, but I'm really open to any options.

ondras commented 6 years ago

As far as backwards compatibility goes, I am totally okay with new major version that changes the API surface a bit (mostly those prototype enhancements). There is not much going on in rot.js these days, so people are not really forced to break compatibility in order to get shiny new things. This discussion is related mostly to geeky inner reworking which does not really improve stuff for a regular consumer.

TypeScript: I like the idea of typed source code. I also like the idea of not having to use a transpiler when doing stuff (modulo Rollup to bundle things), so perhaps there might be some way to make the code vanilla-es6 itself, but with type annotations (jsdoc/similar comment-style)? I recall Flow being a tool for this kind of stuff several years ago...

seanohue commented 6 years ago

With regards to backwards compatibility for prototype enhancements, maybe an opt-in ROT.polyfill() method could be added that adds said enhancements to maintain backwards compatibility with existing game code. (I don't have a vested interest either way, just spitballing)

scambier commented 6 years ago

Any news on this?

ondras commented 6 years ago

Any news on this?

No, unfortunately. My time is a very limited resource these days (leaving for a three-week vacation in four days, btw) and re-working rot.js into ES6 and/or TS is a truly major task.

On the other hand: I am somewhat taking care about quite a large number of projects, all of them require some maintenance, and rot.js is probably the number 1 when it comes to priority. So this refactor is going to happen some day, that is for sure.

ondras commented 5 years ago

Hi there, (/cc @lostfictions @blinkdog @scambier)

I have some good news! The work on TypeScript-based refactor of rot.js has just begun.

My TS skills are still not that high, so I will be learning a lot during the process. You can see the ongoing work in the ts branch (https://github.com/ondras/rot.js/tree/ts).

I moved everything into the _old subdirectory and started with a clean slate. What is done so far:

There is quite a lot to be done and some things are not decided yet:

I am open to discussion. And yes, there won't be no prototype enhancements of built-in objects :tada:

atiaxi commented 5 years ago

Hello!

I'm not sure if it helps, but as a side project I've been maintaining the Typescript bindings for RotJs at DefinitelyTyped. I haven't made any drastic changes or anything but if should at least give an idea on how to structure things :)

lostfictions commented 5 years ago

@ondras That all sounds fantastic!

For docs, my experience with TypeDoc has been that it unfortunately doesn't have great support for richer documentation that includes interactive elements, usage samples in context, etc. There are other docgens nowadays that allow these but also include docstrings and parameter descriptions alongside... I wonder if something like Docz might possibly be a better fit? (I'm also just generally not a fan of TypeDoc's look and feel, though I'm not sure if that can be improved with themes or extra work.)

For testing, I've started using Jest and have only had great experiences with it; it seems like the JS community has been gravitating towards it for a while and that it now has a critical mass of support. (Plus in general it's significantly simplified/standardized my test setups, since it's a runner but also includes assertions, mocks/spies, etc. -- no need for mocha+chai+sinon+whatever.) Most importantly, ts-jest lets you write tests in TypeScript and consume TS files without a transpilation step, so it's not at all cumbersome to set up.

For Node, you could perhaps just run tsc with a Node-specific outDir parameter, and a default entry point different from the one you use for Rollup that exports the Node-specific API? tsc will happily transpile TS files to commonjs (and down to ES5, or alternately ES2016+ if you only want to support the latest LTS version of Node or above).

Hope that helps! Happy to field TypeScript-specific questions or review code, and I could potentially also make a pass at setting up any of those things I just described above if that would be helpful.

lostfictions commented 5 years ago

For .d.ts files I'm not too sure what the question is -- for projects written in TypeScript, there's no need to mess with DefinitelyTyped or anything, you just publish the generated .d.ts files alongside the NPM package and consumers will automatically pick them up. You just need a types field in your package.json alongside the main field -- see here: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

ondras commented 5 years ago

I'm not sure if it helps, but as a side project I've been maintaining the Typescript bindings for RotJs at DefinitelyTyped. I haven't made any drastic changes or anything but if should at least give an idea on how to structure things :)

Great, thanks! I will Definitely have a look ;-)

ondras commented 5 years ago

For docs, my experience with TypeDoc has been that it unfortunately doesn't have great support for richer documentation that includes interactive elements, usage samples in context, etc.

I am completely open to any docgen out there. It should probably support both TS annotations and jsdoc-comment annotations, as the codebase will probably use these two.

For testing, I've started using Jest and have only had great experiences with it; it seems like the JS community has been gravitating towards it for a while and that it now has a critical mass of support. (Plus in general it's significantly simplified/standardized my test setups, since it's a runner but also includes assertions, mocks/spies, etc. -- no need for mocha+chai+sinon+whatever.) Most importantly, ts-jest lets you write tests in TypeScript and consume TS files without a transpilation step, so it's not at all cumbersome to set up.

I would like to stick with Jasmine, because all current rot.js tests are written in Jasmine. But I plan to switch from HTML runner to console runner, so that those tests that are not browser-specific can be run easily.

Some tests are browser-specific, though. That is why I would like to apply them to the transpiled JS code, as opposed to source TS.

For Node, you could perhaps just run tsc with a Node-specific outDir parameter, and a default entry point different from the one you use for Rollup that exports the Node-specific API? tsc will happily transpile TS files to commonjs (and down to ES5, or alternately ES2016+ if you only want to support the latest LTS version of Node or above).

I am okay with ES2015 modules for Node. The issue here are some polyfills/shims that make browser-based APIs (canvas/ROT.Display) available in console apps as well.

Hope that helps! Happy to field TypeScript-specific questions or review code, and I could potentially also make a pass at setting up any of those things I just described above if that would be helpful.

Cool, thanks! I will let you know once I get some more things done.

For .d.ts files I'm not too sure what the question is -- for projects written in TypeScript, there's no need to mess with DefinitelyTyped or anything, you just publish the generated .d.ts files alongside the NPM package and consumers will automatically pick them up. You just need a types field in your package.json alongside the main field -- see here: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

My main issue here is how to structure these files, because the end product is not a single library file, but rather a directory structure of ES2015 modules. Shall I have one .d.ts per one .js, or can these declarations be somehow packed together ... ?

lostfictions commented 5 years ago

I would like to stick with Jasmine, because all current rot.js tests are written in Jasmine. But I plan to switch from HTML runner to console runner, so that those tests that are not browser-specific can be run easily.

Some tests are browser-specific, though. That is why I would like to apply them to the transpiled JS code, as opposed to source TS.

I'd highly recommend taking a look at the Jest docs and some examples: it supports Jasmine's API for test groups, assertions, etc., so (picking an file from the specs directory at random here) something like spec/display.js might work out-of-the-box -- describe("a test group", function() { ... }), it("test description", function() { ... }) and expect(whatever).toBe(something) will all work. (Assuming ROT is in scope or you explicitly add an import of Display at the top of the file, of course.)

It also actually supports a DOM environment out of the box via jsdom -- see this example with JQuery -- or if you actually need to run tests in a full browser I think you could automate that with something like jest-puppeteer too.

I am okay with ES2015 modules for Node. The issue here are some polyfills/shims that make browser-based APIs (canvas/ROT.Display) available in console apps as well.

You might be okay with ES modules for Node, but I'm not sure Node is okay with ES modules :)

Node's support for ESM is experimental and currently has to be enabled with a flag, and it's probably not reasonable to expect any consumer of the library to turn that on, so transpiling is the simplest way to fix that. (And you need to transpile out the Typescript syntax at minimum anyway, so it's not really costing an extra step -- you just declare "module": "commonjs" in your tsconfig.json.)

But yes, the approach I'd recommend is as I said -- a Node-specific entry point file (different from the one you point Rollup towards) that imports whatever shims you need, and then exports the API.

My main issue here is how to structure these files, because the end product is not a single library file, but rather a directory structure of ES2015 modules. Shall I have one .d.ts per one .js, or can these declarations be somehow packed together ... ?

The output is always a single .d.ts file, and Typescript handles this for you. The first step is just to set "declaration": "true" in your tsconfig.json, and then point the "types" files in package.json to wherever the .d.ts file gets generated to. This will get picked up by consumers importing the package via Node, Webpack, Rollup, etc.

(For consumers just loading the module via a script tag in the browser, the declaration file is probably moot, though if they're for some reason using that method and also typescript they can still make a /// <reference ... declaration with the path to the declaration file if needed.)

ondras commented 5 years ago

I'd highly recommend taking a look at the Jest docs and some examples: it supports Jasmine's API for test groups, assertions, etc., so (picking an file from the specs directory at random here) something like spec/display.js might work out-of-the-box -- describe("a test group", function() { ... }), it("test description", function() { ... }) and expect(whatever).toBe(something) will all work. (Assuming ROT is in scope or you explicitly add an import of Display at the top of the file, of course.)

Sounds nice! I will definitely have a look. Note that the tests shall be runnable from a HTML page as well.

The output is always a single .d.ts file, and Typescript handles this for you. The first step is just to set "declaration": "true" in your tsconfig.json, and then point the "types" files in package.json to wherever the .d.ts file gets generated to. This will get picked up by consumers importing the package via Node, Webpack, Rollup, etc.

Will try!

(And you need to transpile out the Typescript syntax at minimum anyway, so it's not really costing an extra step -- you just declare "module": "commonjs" in your tsconfig.json.)

My initial idea was to point nodejs users to (transpiled) ES2015 modules so they can handle them as they need (either using experimental native modules or by transpiling them to commonjs etc). As far as the complicated build chain is considered, the entry point in most cases should be the lib/ directory (TS used only as an authoring format).

I will create a Graphviz image showcasing the various formats and bundles produced by the build process :)

ondras commented 5 years ago

@lostfictions I just ported ROT.Display and boy, that TS is a lot of work! You certainly did quite a lot of work in your TS fork. Kudos to you!

(I am unfortunately not directly re-using your code, as I want to learn TS myself and this is a great opportunity. I am, however, taking a lot of inspiration from your work.)

There is one thing that still puzzles me: with strictNullChecks, this seems to be a problem:

[].concat("a")

I can solve it with

([] as any).concat("a")

but it is not very clean. Moreover, you do not seem to have this issue in your code. Can you please give some insight into this?

atiaxi commented 5 years ago

Not lostfictions but I know this one. To pick a line of code out of rect.ts:

let chars = ([] as any).concat(ch);

The easiest thing to do is change the any to the actual type of the array:

let chars = ([] as string[]).concat(ch);

I'm guessing a bit at what you're doing here; it looks like ch can either be a string or a string[] and this line converts it to an array of strings no matter what. A neat feature of Typescript is Type Guards which would let you write the above a bit more cleanly:

let chars: string[];
if(typeof(ch) === 'string') {
  chars = [ch];
} else {
  chars = ch;
}
lostfictions commented 5 years ago

Right -- both these approaches work. In general this isn't an issue because typically one concats something into a non-empty array -- that's the whole point of concatting, right? -- but this is a case that confuses the typechecker. When you declare an empty array without a type hint, it has no context to infer what kind of thing(s) are going to go in there, so it defaults to never[] (nothing can go in there!) and any concat operation will fail, even if it immediately follows. So generally you want to either initialize the array with data, or if you declare an empty array annotate what's going to go in there.

Incidentally, TS can usually infer this, too -- I think @atiaxi's second snippet can also simply be written without the annotation for chars:

let chars;
if(typeof ch === 'string') {
  chars = [ch];
} else {
  chars = ch;
}

or even more concisely:

const chars = typeof ch === 'string' ? [ch] : ch
// or
const chars = Array.isArray(ch) ? ch : [ch]

(Assuming Typescript knows that ch is either string or string[] with no other options.)

Often for clarity and documenting your assumptions it's nice to have an explicit annotation, though.

lostfictions commented 5 years ago

Incidentally, for what it's worth, you should never have to use any in your own code, even though it may seem tempting when you're getting started.

It may sometimes be useful at the boundaries of your codebase where you're interacting with someone else's code, but even then you can typically do better than any (unknown is usually a better option if nothing more specific applies). In shared TS codebases I like to turn on a no-explicit-any lint rule so that folks only use it if they're really, really sure they need it and can explain why :)

atiaxi commented 5 years ago

Often for clarity and documenting your assumptions it's nice to have an explicit annotation, though.

Another reason I tend to do exactly that even when the compiler has it in hand is that I find it makes the Autocomplete work better :)

ondras commented 5 years ago

Thanks for all the comments. I decided to stick with ([] as string[]).concat, although in my opinion it makes the code much less readable. I consider [].concat(x) as an idiomatic way of converting a value to array in JS...

ondras commented 5 years ago

The output is always a single .d.ts file, and Typescript handles this for you. The first step is just to set "declaration": "true" in your tsconfig.json, and then point the "types" files in package.json to wherever the .d.ts file gets generated to. This will get picked up by consumers importing the package via Node, Webpack, Rollup, etc.

@lostfictions this does not seem to work for me. TS generates one .d.ts per each .js generated. I suppose this is okay, as long as the end user opts to use ES2015 modules. But those two prefer the bundled file will have to live without .d.ts.

ondras commented 5 years ago

Slowly moving forwards.

ondras commented 5 years ago

I am almost done with the refactor. Some remarks before the work is completely finished:

/cc @lostfictions @blinkdog

blinkdog commented 5 years ago

I am almost done with the refactor. Some remarks before the work is completely finished:

* there is no separate _node_ version of rot.js; the library simply contains Node support out of the box. The most problematic part (ROT.Display) has been refactored in a way that no longer requires shims and/or polyfills.

That's pretty cool. Way back when I first wrote the shims, my goal was to make the library usable in Node with minimal disruption to mainline development. Today, Node lives in the mainline.

* CoffeeScript tests are probably not going to make it. My knowledge of CS is limited and I do not have the energy to refactor them as well. Some might _just work_, but I will leave this up to someone else.

* I decided to stick with Jasmine for unit tests. I added a puppeteer-based runner that allows running of tests without a regular browser. Just `make test` and see.

I can scope this work out after November. I do think there is value in maintaining a coverage suite, but I'd probably aim to refactor the CoffeeScript tests as ES6 tests running under Jasmine. I think istanbul can use Jasmine as a test runner, so we can still get sweet coverage reports.

lostfictions commented 5 years ago

One problematic part to port is the ROT.DEFAULT_WIDTH / ROT.DEFAULT_HEIGHT value. ES6 modules allow only read-only exports, but there is existing code (including the interactive manual) that modifies these values. One way to solve this would be to just export ROT.DEFAULT_SIZE -- as a constant, but with mutable members. Not very pretty though.

ES modules just export immutable bindings to live values, so something like this is definitely possible:

export let DEFAULT_WIDTH = 40;
export function setDefaultWidth(newWidth: number) {
    DEFAULT_WIDTH = newWidth;
}

So DEFAULT_WIDTH won't be writable for consumers of the module, but setDefaultWidth will work just fine. (This article has a nice summary of that if it helps.) You could maybe prefix any exported-but-internal functions like that with underscores, eg. export function __setDefaultWidth to make it a bit more obvious if they're not intended for regular use.

If it's otherwise a question of being confusing for defaults to be writable at all after program initialization (since they're only changed for internal use, sounds like?) I'm sure rollup has something equivalent to webpack's DefinePlugin -- maybe you could leverage that to set defaults for things like the interactive manual, if it doesn't need to be configured dynamically/more than once?

ondras commented 5 years ago

I suppose the problem here is that there is no well-defined use case for these values. They are used as sane defaults in several places, where map size is optional. I can surely find out a workable solution for the interactive manual (such as using the size explicitly), but I have no idea whether people are actually used to changing these values throught their codebase. Sure, any solution here will be backwards-incompatible, but I am trying to find out how large the newly-created incompatibility should be.

ondras commented 5 years ago

Done. The TS codebase has been merged into master.

The TS part is now finished, but the modernizing is not: we have Classes and let/const statements, but the code still needs some cleanup and/or modernization. This can be done on-the-fly in small incremental changes.

bbugh commented 5 years ago

@ondras exciting for TypeScript! Will you be publishing the types with the package? Currently the source is using TypeScript but we can't import types from it without still using @types/rot-js (which has some problems).

bbugh commented 5 years ago

Apologies, looks like you've already set up the types, but not in a way that the TypeScript compiler can automatically find them. I created a simple PR https://github.com/ondras/rot.js/pull/142 so that our projects can pick it up.