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

Dependent interface SpeedActor is not exported #144

Closed bbugh closed 5 years ago

bbugh commented 5 years ago

Thanks for rebuilding this in TypeScript, it's really great to have a mature library with types. I imagine it was a big job.

Current Behavior

The Speed Scheduler has an interface SpeedActor, expecting T to extend SpeedActor. Unfortunately, SpeedActor isn't exported:

https://github.com/ondras/rot.js/blob/f62ac1ed9fd9f06973cd351037a5daed9cc3ed1b/lib/scheduler/speed.d.ts#L2-L5

We can work around it by defining our own compatible interface base type with getSpeed, but then we're hoping it matches the contract rather than guaranteeing it.

Expected Behavior

SpeedActor is exported so we can derive types from it safely.

ondras commented 5 years ago

I am not 100% sure how to do this.

I can simply add export to the SpeedActor interface, but this module is re-exported in the sibling index.js file. And it is apparently not possible to re-export an interface this way...

bbugh commented 5 years ago

Yeah, I didn't submit a PR because there's more to consider.

I think with the given setup (Scheduler being an exported namespace rather than each individual scheduler being exported), it's a little tricky. I see that other modules have partial exports too (e.g. StringGenerator is exported without Options).

Typically, a library's index.ts will export all of the library modules at the top level (without namespaces), which allows the end user to decide how to import things, like so:

// Expected
import { SpeedScheduler as SomeOtherName, SpeedActor } from 'rot-js'
new SomeOtherName

// Actual
import { Scheduler, SpeedActor } from 'rot-js'
new Scheduler.Speed // maybe I have a Speed type, and I can't alias this one so it gets confusing

I believe the current kind of namespace importing also reduces the optimization of tree-shaking. To implement that, you'd do something like this:

diff --git a/lib/index.d.ts b/lib/index.d.ts
index 392f900..0b2b35f 100644
--- a/lib/index.d.ts
+++ b/lib/index.d.ts
@@ -2,7 +2,10 @@ export { default as RNG } from "./rng.js";
 export { default as Display } from "./display/display.js";
 export { default as StringGenerator } from "./stringgenerator.js";
 export { default as EventQueue } from "./eventqueue.js";
-export { default as Scheduler } from "./scheduler/index.js";
+export { default as Scheduler } from './scheduler/scheduler'
+export { default as SpeedScheduler, SpeedActor } from './scheduler/speed'
+export { default as SimpleScheduler } from './scheduler/simple'
+export { default as ActionScheduler } from './scheduler/action'
 export { default as FOV } from "./fov/index.js";
 export { default as Map } from "./map/index.js";
 export { default as Noise } from "./noise/index.js";

However, given that you just launched a major version for the TypeScript library and a change along those lines would require a semver major version change, perhaps the simplest options is better - just export SpeedActor at the top level. (This doesn't include the unexported StringGenerator, Lighting interfaces and so forth.)

diff --git a/src/index.ts b/src/index.ts
index ecfbbb8..caa259c 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -2,7 +2,7 @@ export {default as RNG} from "./rng.js";
 export {default as Display} from "./display/display.js";
 export {default as StringGenerator} from "./stringgenerator.js";
 export {default as EventQueue} from "./eventqueue.js";
-export {default as Scheduler} from "./scheduler/index.js";
+export {default as Scheduler, SpeedActor} from "./scheduler/index.js";
 export {default as FOV} from "./fov/index.js";
 export {default as Map} from "./map/index.js";
 export {default as Noise} from "./noise/index.js";
diff --git a/src/scheduler/index.ts b/src/scheduler/index.ts
index daa8a5a..5531d6b 100644
--- a/src/scheduler/index.ts
+++ b/src/scheduler/index.ts
@@ -2,4 +2,6 @@ import Simple from "./simple.js";
 import Speed from "./speed.js";
 import Action from "./action.js";

+export { SpeedActor } from './speed'
+
 export default { Simple, Speed, Action };
diff --git a/src/scheduler/speed.ts b/src/scheduler/speed.ts
index c5444d9..10e2188 100644
--- a/src/scheduler/speed.ts
+++ b/src/scheduler/speed.ts
@@ -1,6 +1,6 @@
 import Scheduler from "./scheduler.js";

-interface SpeedActor {
+export interface SpeedActor {
    getSpeed: () => number;
 }
ondras commented 5 years ago

Thanks @bbugh for a thorough explanation. I fixed the export in the way you suggested.