-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
Unable to use ROT 2.0.2 #148
Comments
For node, please use the |
Hm. This should work then. Yet it completely borks my TypeScript compiler. The code I used was something along these lines:
The compiler complains about the namespace |
Can you find out whether this resolves to 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 |
I'll try to determine that after work today. Using the contents of 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. |
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:
Initial attempt to compile it (again, using rot.js 2.0.2 from npm) yields an error:
This is the issue I submitted a PR for. After fixing the import in
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 I can offer some assistance with this if needed. |
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 Lines 41 to 42 in ab046a2
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). |
@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. |
@lostfictions My guess would be that type declarations and closure compiler have nothing to do with each other. Type declarations are produced by tsc, Line 7 in ab046a2
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:
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 |
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.
It is a pre-built version wher you just want to download and include in your <script> tag. I am not sure why are we discussing this particular build artifact in this thread, since it is definitely not the right file for node.js usage. And now for individual comments:
I see. In my opinion the That is why the package.json's It would be great if you convinced your TS to use the
For me,
Not sure how CC got into the discussion, but well... The general idea is:
The minification phase is only present for people without a build toolchain that want to just
It is definitely acceptable (well at least I am open to suggestions). But I am still missing the crucial information about what type of build artifacts are you interested in (i.e. what is currently missing in the rot.js package).
Yes! They are the code you want to use when developing a game with rot.js. They are the way we structure our (client-side) JS code now in 2018; they are what I feed my build process with. You can use them directly in the browser, you can bundle them (no babel is actually required), you can integrate them into your oh-such-webpack build process as requested. Please note that I am not inventing a wheel here; other library projects are often doing something very similar: https://github.com/d3/d3/blob/master/package.json
Good point, but only for node.js usage. If you want this step, just transpile the current
This is what already happens (just with rollup instead of browserify).
See https://github.com/rollup/rollup/wiki/pkg.module for reference. |
After giving this some more thought (and consulting with the current build schema), I am now almost certain I understand your needs. You want the green part, except that the files shall be using CommonJS instead of ES6 modules. Am I correct? |
That leaves the 20% of folks like me who will inevitably be dissatisfied.
Not criticising Closure's output. I'm just saying it's more natural to replace it with UglifyJS, which does the exact same thing, but without needing a Java tool in the process. So condition 2a is fulfilled, and condition 2b is irrelevant in the context of the end user. The end user needs zero effort to build the project. However, as a developer, I'd prefer to use a toolchain that requires I only type Oh, also, Closure has an npm wrapper that also bundles with a native JS compiler: https://www.npmjs.com/package/google-closure-compiler - so even if it actually produces code that's smaller than UglifyJS, there's no excuse for not using Node.js.
TL;DR: this is not doable.
I'm afraid this is entirely on rot.js side to make sure it "just works" in any environment, be it browser of Node.js.
The "for me" part is the very definition of an idiosyncratic build. It's the build system for you, not for the majority of JavaScript/TypeScript developers.
It's got ES2017 modules which are completely useless in an npm package. Instead, it needs standard CommonJS code.
Provided you use a module bundler at all, and provided that's specifically Rollup. Mind you, Webpack is much more popular AFAIK. Webpack has 4.3M weekly downloads, rollup has 421k. That's an order of magnitude difference. For comparison, Browserify is 510K. These seem to be the only bundlers that count nowadays (there are some exotic ones too, like Fuse-Box or Parcel, but they have barely any user base), and Rollup is the least popular of the three. Also, as stated before, modules are completely useless in an npm package. They cannot be used because
And the browser usage requires another step in the same toolchain. Also, I'm repeating myself, but: I do not with to transpile anything myself. That's the whole point of an npm package. It's not meant to be transpiled.
I fail to see your point. Rollup allows you to specify the
See the part in bold. I'm using Node.js, not coding for a browser and therefore not using Rollup. Also, again, what about other module bundlers? |
In a nutshell, yes. This would solve my issues as the end user. This + making As a potential contributor to rot.js though, I find that the build toolchain is overly exotic for me to even bother. |
Also, I have an unrelated question (sorry to pollute the topic, but I really don't want to open a new issue just to ask this): what is |
You mean RecursiveShadowcasting? That would be #29 . |
Sorry, I meant |
My point here is that the published package is oriented towards the 80%, i.e. it closely mirrors their needs. This does not mean that 20% are completely out of the game, but they are not the main focus. I am, however, (as you can see in this discussion) trying to provide a solution acceptable to them as well.
That is precisely the package I am using.
Right, nobody is expecting you to.
Yeah, the whole
Well, this is kinda the status quo with JavaScript -- it almost never just works. We have a plethora of runtimes, so we have to carefully pick and act accordingly, typically using transpilation. Your server-side code is also bound to some language version; if you wish to support older node.js versions, you have to transpile anyway. This is something so common in client-side JS that it looks very natural to me, even server-side. That is why I recommend transpiling/bundling the server-side code as well.
I am basically certain that all popular bundlers support ES2015 modules. Do you have an evidence that this is not the case?
I would be more than happy to adjust the output as neccessary, provided I understand what exactly is being requested. I currently see two ways of solving the Node problem:
In other words, having |
Ah, that is my own algorithm. See http://www.roguebasin.roguelikedevelopment.org/index.php?title=Precise_Shadowcasting_in_JavaScript for reference. |
Oh, I was sure you were calling the Java application. Forget I said anything then.
You contradict yourself, dear sir.
No, this is typically not done server-side. Also, let me repeat my main source of grief: an npm package does not expose the
No evidence, since I don't use them. AFAIK, Browserify does not, and Webpack only supports them in versions 2+. What I don't know and highly doubt is whether they also support the
OK, there are two issues discussed here, so let's make this crystal clear:
Node.js does not care whether it's a set of separate files with transpiled TS or a single bundled file. Also, the bundled file works both in Node.js and the browser due to the special treatment of the Lines 5 to 7 in ab046a2
So |
My point here was the concept "make any changes to npm modules that I install", in a way that would actually modify the installed npm module. This obviously sounds like a bad idea and I agree that it should not be done. I considered bundling/transpilation as an operation that does not modify the installed module. Probably just a miswording from my side. Ignore me.
Great! Please show me how to adjust the TS typing so that the
Browserify: plugin |
OK, so we're down to the very essence of this post: fixing the typings in order to get Node + TS up and running with rot.js. I will have a look at the code and see if I can point out exactly what changes would be required. |
Alright, it seems I cannot make it into anything meaningful. I wanted to reorganise the exports so that this code would be valid:
However, it would appear that TypeScript will not allow such namespace nesting. This code is valid:
so is this:
The only improvement I can think of is eliminating default imports so that upon typing Otherwise, feel free to close this issue. |
Well, we can probably still try the approach with a node-specific build. Such directory would contain individual .js files with .d.ts declarations and commonjs module system. Do you wish to try how that would work for you? |
Might be relevant: microsoft/TypeScript#4433 |
No. There's no need to provide a separate CommonJS set of files. As I said before, Node.js is more than happy to use a bundled file, so that's not an issue. The |
This is something that interests me: how does this even work? The bundle is |
TL;DR: he location of
Line 30 in ab046a2
When imported via npm, whenever you Types are also referenced for TS projects: Line 64 in ab046a2
So, whenever you use the same import, |
Ah, I basically forgot about the |
Because they are not named. When importing a default export, you give it your own name. You can import it as |
My TS gives working intellisense for properties of a default export (named during import). I am not sure whether this is the same case, but this works: import XYZ from "./file-with-default-export.js";
XYZ.doStuff(); // <--- doStuff is correctly intellisensed |
No, this is not the same use case. Once you import an object, TypeScript will know what it's referencing. Using something that's not yet been imported is harder. |
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.The text was updated successfully, but these errors were encountered: