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

Fix implicit any type for Scheduler.next()/remove() #145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix implicit any type for Scheduler.next()/remove() #145

wants to merge 1 commit into from

Conversation

bbugh
Copy link
Contributor

@bbugh bbugh commented Nov 29, 2018

Addressed Issues

Scheduler<T> takes a type, but its internal representation of the object tracker (_current) was set to any instead of T. This resulted in next() returning any, which lost typing and required unnecessary casting when it is called.

This also creates an error that could be caught at compile time, as next() can also return null, which is not enforced by the type checker.

Lastly, remove(thing: any) lost the compiler-time checking when calling it.

Fixes

This fixes next() and remove() to use the expected T so that the type checker catches all of these issues with all of the schedulers.

I didn't run the make process with the PR because I'm unclear if you do that yourself, but I'm happy to push another commit with doc/lib built.

Example

From the documentation, here's an example of where this was going wrong:

interface SpeedyActor {
  number: number
  getSpeed: () => number
}

const scheduler = new Scheduler.Speed<SpeedyActor>()

/* simulate several turns */
const turns = []
for (let i = 0; i < 40; i++) {
    const current = scheduler.next() // without this PR, `current` is `any`
    // `current` can be null, but this isn't type check enforced
    turns.push(current.number) // `current` not type checked during usage either
}

scheduler.remove("bad entry") // should be a typeof SpeedyActor but it will take anything

These issues are fixed by the PR.

Scheduler<T> takes a type, but its internal representation of the object
tracker was set to `any` instead of `T`. This resulted in `next()`
returning `any`, which lost typing and required unnecessary casting when
it is called..

This also creates an error, as `next()` can also return null, which is not enforced by the type checker.

Lastly, `remove()` item was `any`, rather than `T`, so you lost the
type checking for calling it.

This fixes `next()` and `remove()` to use the expected `T` so that the
type checker catches all of these issues with all of the schedulers.
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

Successfully merging this pull request may close these issues.

1 participant