Skip to content
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

Open
mingos777 opened this issue Dec 16, 2018 · 30 comments
Open

Unable to use ROT 2.0.2 #148

mingos777 opened this issue Dec 16, 2018 · 30 comments

Comments

@mingos777
Copy link
Contributor

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
Copy link
Owner

ondras commented Dec 16, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

ondras commented Dec 17, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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:

rot.js/Makefile

Lines 41 to 42 in ab046a2

watch: all
while inotifywait -e MODIFY -r src; do make $^ ; done

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

mingos777 commented Dec 18, 2018

@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:

"declaration": true,

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
Copy link
Owner

ondras commented Dec 18, 2018

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 <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:

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

I see. In my opinion the lib/ version is a better one, because its contents are accompanied by individual .d.ts files with type definitions. The dist/ version is basically type-less. Also, the dist/ files were processed by babel, which makes them more pre-cooked for easy usage. Power users might want to supply their own build process, using the lib/ instead.

That is why the package.json's module field points to lib/index.js; ES6-module-aware toolchains are supposed to prefer this (instead of main).

It would be great if you convinced your TS to use the lib/ version.

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 me, Make is the build system of choice, presenting no peculiarities and/or idioidiosyncrasies. I am 100% sure that the build process itself is not to blame here -- perhaps the repo is badly organised, perhaps the exports are structured incorrectly, perhaps the tools are misconfigured. But this does not relate to the build orchestration.

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).

Not sure how CC got into the discussion, but well...

The general idea is:

  • no-building-fast-setup: pick dist/*.js and go,
  • build yourself as necessary: pick lib/index.js

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.

The minification phase is only present for people without a build toolchain that want to just <script src=dist/rot.min.js>. You do not want to pick this file, nor are you required to use the java-based tool for your minification needs.

@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.

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).

Also, are the ES2017 modules used for anything apart from feeding Babel with them?

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

* 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.

Good point, but only for node.js usage. If you want this step, just transpile the current lib/ directory from es2015 modules to commonjs modules, right?

* 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.

This is what already happens (just with rollup instead of browserify).

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.

See https://github.com/rollup/rollup/wiki/pkg.module for reference.

@ondras
Copy link
Owner

ondras commented Dec 18, 2018

@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.

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).

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?

@mingos777
Copy link
Contributor Author

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.

That leaves the 20% of folks like me who will inevitably be dissatisfied.

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.

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 npm install, no extra tools.

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.

It would be great if you convinced your TS to use the lib/ version.

TL;DR: this is not doable.

  1. I do not wish to make any changes to npm modules that I install. The whole idea behind using npm is not having to do any alterations or manual building of the installed packages. They must work out of the box.
  2. Node.js does not understand ES2017 modules. As stated before, there's is a possibility to use them with the aid of the --experimental-modules flag, but not passing this flag will make the whole program fail and also, Node.js only recognises modules with .mjs file extension, which the JS files in lib/ do not have.

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.

For me, Make is the build system of choice

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.

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).

It's got ES2017 modules which are completely useless in an npm package. Instead, it needs standard CommonJS code.

Yes! They are the code you want to use when developing a game with rot.js.

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 package.json doesn't point to them, and even if it would, they would never work on today's Node.js runtime. This may change in the future, but the current LTS (v10) is not able to use these files.

Good point, but only for node.js usage. If you want this step, just transpile the current lib/ directory from es2015 modules to commonjs modules, right?

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.

See https://github.com/rollup/rollup/wiki/pkg.module for reference.

I fail to see your point. Rollup allows you to specify the "module" property. The doc you linked says:

Now we're cooking with gas - my-package continues to work for everyone using legacy module formats, but people using ES2015 module bundlers such as Rollup don't have to compromise. Everyone wins!

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?

@mingos777
Copy link
Contributor Author

You want the green part, except that the files shall be using CommonJS instead of ES6 modules. Am I correct?

In a nutshell, yes. This would solve my issues as the end user. This + making package.json point to lib/index.js (not the "module" property; Node won't be able to use that).

As a potential contributor to rot.js though, I find that the build toolchain is overly exotic for me to even bother.

@mingos777
Copy link
Contributor Author

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 RestrictiveShadowcasting based on? Is it something you came up with yourself, or an adaptation of existing code? Initially I thought it could be MRPAS, but the FOV shape, performance and symmetry are wildly different between the two. The code does not seem like even a loose adaptation, so... what is it?

@ondras
Copy link
Owner

ondras commented Dec 18, 2018

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 RestrictiveShadowcasting based on?

You mean RecursiveShadowcasting? That would be #29 .

@mingos777
Copy link
Contributor Author

Sorry, I meant PreciseShadowcasting.

@ondras
Copy link
Owner

ondras commented Dec 18, 2018

That leaves the 20% of folks like me who will inevitably be dissatisfied.

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.

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.

That is precisely the package I am using.

.

1. I do not wish to make any changes to npm modules that I install. The whole idea behind using npm is **not** having to do any alterations or manual building of the installed packages. They **must** work out of the box.

Right, nobody is expecting you to.

2\. Node.js does not understand ES2017 modules. As stated before, there's is a possibility to use them with the aid of the `--experimental-modules` flag, but not passing this flag will make the whole program fail and also, Node.js only recognises modules with `.mjs` file extension, which the JS files in `lib/` do not have.

Yeah, the whole .mjs sounds like a bad idea to me.

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.

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.

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.

I am basically certain that all popular bundlers support ES2015 modules. Do you have an evidence that this is not the case?

As a potential contributor to rot.js though, I find that the build toolchain is overly exotic for me to even bother.

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:

  1. generating a suitable typed metadata for the existing dist/rot.js bundle, so TS people can leverage the types, OR

  2. generating another set of TS-processed files, in commonjs format, exclusively for node.js.

In other words, having dist-browser/ and dist-node/, because apparently these both need special care when they do not want to have a build process. But only one of these can be referenced from the package.json's main field.

@ondras
Copy link
Owner

ondras commented Dec 18, 2018

Sorry, I meant PreciseShadowcasting.

Ah, that is my own algorithm. See http://www.roguebasin.roguelikedevelopment.org/index.php?title=Precise_Shadowcasting_in_JavaScript for reference.

@mingos777
Copy link
Contributor Author

That is precisely the package I am using.

Oh, I was sure you were calling the Java application. Forget I said anything then.

Right, nobody is expecting you to.

That is why I recommend transpiling/bundling the server-side code as well.

You contradict yourself, dear sir.

This is something so common in client-side JS that it looks very natural to me, even server-side.

No, this is typically not done server-side. Also, let me repeat my main source of grief: an npm package does not expose the lib/ directory at all, so even if it was CommonJS, it would still be unavailable to Node.js.

I am basically certain that all popular bundlers support ES2015 modules. Do you have an evidence that this is not the case?

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 "module" property of package.json. Maybe you know the answer.

I would be more than happy to adjust the output as neccessary, provided I understand what exactly is being requested.

OK, there are two issues discussed here, so let's make this crystal clear:

  1. I'm requesting a working set of TS type declarations. These are required to make the npm package usable. The change here is TS-specific and requires a rework of the exports so that the generated typings allow the end user to use the ROT object without having to import individual files, as shown in the test code I pasted a number of posts earlier. This is the change I offered help with.

  2. I'm suggesting a modification of the build toolchain to just use Node.js. If you're determined to use Make, it's your choice and if you say "no", it's your right to do so. This is not required for anything, just a suggestion that would allow contributors to just jump in without figuring out the weird build process. I could help with this, but only provided such help is required. From what I can tell, it's not, so this is probably an issue we can consider closed.

But only one of these can be referenced from the package.json's main field.

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 exports object:

rot.js/dist/rot.js

Lines 5 to 7 in ab046a2

(function (global, factory) {
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) : typeof define === 'function' && define.amd ? define(['exports'], factory) : factory(global.ROT = {});
})(this, function (exports) {

So package.json should still point to dist/rot.js.

@ondras
Copy link
Owner

ondras commented Dec 18, 2018

Right, nobody is expecting you to.

That is why I recommend transpiling/bundling the server-side code as well.

You contradict yourself, dear sir.

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.

1. I'm **requesting** a working set of TS type declarations. These are required to make the npm package usable. The change here is TS-specific and requires a rework of the exports so that the generated typings allow the end user to use the `ROT` object without having to import individual files, as shown in the test code I pasted a number of posts earlier. This is the change I offered help with.

Great! Please show me how to adjust the TS typing so that the dist/rot.js file is usable for you.

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 "module" property of package.json. Maybe you know the answer.

Browserify: plugin
Webpack: the module field was requested back in 2016 and is probably implemented, even though the discussion/commit is not really readable to me.

@mingos777
Copy link
Contributor Author

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.

@mingos777
Copy link
Contributor Author

mingos777 commented Dec 18, 2018

Alright, it seems I cannot make it into anything meaningful.

I wanted to reorganise the exports so that this code would be valid:

import * as ROT from "rot-js";
const fov: ROT.FOV.PreciseShadowcasting = new ROT.FOV.PreciseShadowcasting(lightPassesFunc);

However, it would appear that TypeScript will not allow such namespace nesting. This code is valid:

import * as ROT from "rot-js";
import PreciseShadowcasting from "rot-js/lib/fov/precise-shadowcasting";
const fov: PreciseShadowcasting = new ROT.FOV.PreciseShadowcasting(lightPassesFunc);

so is this:

import PreciseShadowcasting from "rot-js/lib/fov/precise-shadowcasting";
const fov: PreciseShadowcasting = new PreciseShadowcasting(lightPassesFunc);

The only improvement I can think of is eliminating default imports so that upon typing PreciseShadowcasting, you get an actual intellisense hint and automatic import. I'll drop a PR.

Otherwise, feel free to close this issue.

@ondras
Copy link
Owner

ondras commented Dec 19, 2018

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?

@ondras
Copy link
Owner

ondras commented Dec 19, 2018

Might be relevant: microsoft/TypeScript#4433

@mingos777
Copy link
Contributor Author

Do you wish to try how that would work for you?

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 .d.ts files provide plenty information to allow code discoverability - that is, if exports are reworked towards that.

@ondras
Copy link
Owner

ondras commented Dec 19, 2018

The .d.ts files provide plenty information to allow code discoverability - that is, if exports are reworked towards that.

This is something that interests me: how does this even work? The bundle is dist/rot.js, type declarations are lib/subdir/file.d.ts; how is TS able to correctly pair these? Also, the fact that intellisense is failing for you in some scenarios suggests that perhaps this pairing is not flawless.

@mingos777
Copy link
Contributor Author

TL;DR: he location of .d.ts files relative to the .js counterparts is completely irrelevant. It is also irrelevant whether type declarations span a different number of files than .js.

package.json points to the main file:

"main": "dist/rot.js",

When imported via npm, whenever you import * as ROT from "rot-js";, this will always resolve to this dist/rot.js file.

Types are also referenced for TS projects:

"types": "./lib/index.d.ts"

So, whenever you use the same import, tsc will look at lib/index.d.ts to discover the types. Since the index links to more typings files using relative paths, TypeScript will happily traverse them too. In fact, if you wanted to, you could manually join all type declarations into one file or split them so that you have exactly one import per file (like, maybe export a class and an interface or type separately even though they are exported from the same .ts file). TypeScript couldn't care less. It will just traverse the provided declarations structure and assume it corresponds with the indicated .js file.

@ondras
Copy link
Owner

ondras commented Dec 19, 2018

Types are also referenced for TS projects:

Ah, I basically forgot about the types field. My bad. Makes sense now, although I do not really understand why the default exports are not properly discovered.

@mingos777
Copy link
Contributor Author

mingos777 commented Dec 19, 2018

Because they are not named. When importing a default export, you give it your own name. You can import it as RecursiveShadowcasting or as Tiddlywinks (this is different from aliasing!), it's entirely your call. So the IDE does not and should not know what name to suggest in intellisense.

@ondras
Copy link
Owner

ondras commented Dec 19, 2018

Because they are not named. When importing a default export, you give it your own name. You can import it as RecursiveShadowcasting or as Tiddlywinks (this is different from aliasing!), it's entirely your call. So the IDE does not and should not know what name to suggest in intellisense.

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

@mingos777
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants