[discussion] Modernizing rot.js #132
Comments
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 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.
No, definitely not.
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 can speak only for The Rhino-related stuff was an attempt to run said tests using a CLI.
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. |
The 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 @statico liked the work and suggested an official Node release for the rot.js project. I was happy to help make that happen: After that, I decided to write a full coverage test suite for the entire library. The tests ended up under I was very happy with the results of this coverage suite. We killed a ton of tiny bugs. Anyway, hopefully that clears up the test overlap thing. If you have any questions and/or would like some help, just let me know. |
|
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. |
|
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. |
|
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. |
|
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... |
|
With regards to backwards compatibility for prototype enhancements, maybe an opt-in |
|
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. |
|
Hi there, 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 I moved everything into the
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 🎉 |
|
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 :) |
|
@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 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. |
|
For |
Great, thanks! I will Definitely have a look ;-) |
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.
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 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.
Cool, thanks! I will let you know once I get some more things done.
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 |
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 -- 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.
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 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.
The output is always a single (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 |
Sounds nice! I will definitely have a look. Note that the tests shall be runnable from a HTML page as well.
Will try!
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 I will create a Graphviz image showcasing the various formats and bundles produced by the build process :) |
|
@lostfictions I just ported (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 [].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? |
|
Not lostfictions but I know this one. To pick a line of code out of rect.ts: The easiest thing to do is change the I'm guessing a bit at what you're doing here; it looks like |
|
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 Incidentally, TS can usually infer this, too -- I think @atiaxi's second snippet can also simply be written without the annotation for 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 Often for clarity and documenting your assumptions it's nice to have an explicit annotation, though. |
|
Incidentally, for what it's worth, you should never have to use 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 |
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 :) |
|
Thanks for all the comments. I decided to stick with |
@lostfictions this does not seem to work for me. TS generates one |
|
Slowly moving forwards.
|
|
I am almost done with the refactor. Some remarks before the work is completely finished:
|
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.
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 |
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 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? |
|
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. |
|
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. |
|
@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 |
|
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 #142 so that our projects can pick it up. |
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:
rot.jsandrot.min.jsthat don't require using the Node ecosystem, too.)node/test/and the tests intest/. 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 ofnode/test/through the excellent decaffeinate tool in my fork as a first step towards consolidation.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.
The text was updated successfully, but these errors were encountered: