Open whs opened 4 years ago
Awesome, thanks!
Do you think you could add some kind of automated test for the types? It doesn't have to be comprehensive for now, but something using tsd
or at least node-ts
to get it started.
I found out that the EventEmitter3 types require EventEmitter3 4.0 to be able to typecheck event handlers.
Is it ok for me to bump? The only relevant breaking changes is in v3.0.0 and considering that we export the EventEmitter interface we might need to release as semver-major as well.
I think as long as we tossed a setMaxListeners
noop function on the emitter, we could update the event emitter library without a breaking change in abstract-state-router.
Just a quick update, I'm still waiting on https://github.com/SamVerschueren/tsd/issues/45 , seems that adding types to the root will make it check node_modules as well.
I bumped some dependencies in ebd33d9 which fixed that particular issue.
Is there anything left to do?
I don't have anything else to add, so let's merge it
If eventemitter is considered part of the api, then there is one more breaking change. The interface of EventEmitter.prototype.listeners()
has changed slightly. Any outside consumers using the exists
param can no longer use it.
- emitter.listeners(eventName, exists)
+ emitter.listeners(eventName)
As far as breaking changes go, this is about as minor as it gets.
I think it is better just to bump semver-major? I think it is better than monkeypatching the libraries we use to emulate compatibility.
I don't have time to review this .d.ts
file thoroughly, but I did quickly compare it to a version I have been using locally, and I noticed a few differences. The differences noted are based upon my interpretation of the source code and readme.
type Parameter
accepts any
values, but really it can only be string | number | null
stateName
can be null in the go
functionBelow is a dump of my version. I think it has some good features that could be incorporated in this version, like splitting the query string parameters out to their own types.
However, I only know the basics of Typescript, so my version will be lacking in other ways. For example, I don't know how to inform TS that the data
object added to the state was the same one received by the resolve
function. Looking at this code, it seems adding the <DataType>
to the interface might allow that functionality, so I will now go and test that theory!
declare module "abstract-state-router" {
import { EventEmitter } from "eventemitter3"
export default StateProvider
function StateProvider(
makeRenderer: IMakeRenderer,
rootElement: Element,
stateRouterOptions?: StateRouterOptions
): StateProviderEmitter
export interface IMakeRenderer {
(stateRouter: StateProviderEmitter): IRenderer
}
export class StateRouterOptions {
pathPrefix?: string
router?: any
throwOnError?: boolean
}
export interface IRenderer {
render(
context: RendererRenderContext,
callback: IResolveCallack
): DomApi
reset(
context: RendererResetContext,
callback: IResolveCallack
): void | DomApi
destroy(domApi: DomApi, callback: IResolveCallack): void
getChildElement(domApi: DomApi, callback: IResolveCallack): Element
}
class RendererRenderContext {
template: StateTemplate
element: Element
content: StateResolveContent
parameters: QueryStringParameters
}
class RendererResetContext {
domApi: DomApi
content: StateResolveContent
template: StateTemplate
parameters: QueryStringParameters
}
type DomApi = any
export interface IResolveCallack {
(err: any): void
(err: Falsy, content: StateResolveContent): void
redirect(
stateName: string,
stateParameters?: QueryStringParameters
): void
}
export class StateProviderEmitter extends EventEmitter {
addState(state: State): void
go(
stateName: string | null,
stateParameters?: QueryStringParameters,
options?: { replace?: boolean; inherit?: boolean }
): void
evaluateCurrentRoute(
stateName: string,
stateParameters?: QueryStringParameters
): void
makePath(
stateName: string | null,
stateParameters?: QueryStringParameters,
options?: { inherit?: boolean }
): string
getActiveState(): { name: string; parameters: QueryStringParameters }
stateIsActive(
stateName: string,
stateParameters?: QueryStringParameters
): boolean
}
export class State {
name: string
route: string
defaultChild?: string | (() => string)
data?: StateData
template: StateTemplate
resolve?: (
data: StateData,
parameters: QueryStringParameters | any,
callback: IResolveCallack
) => any
activate?: (context: StateActivateContext) => void
querystringParameters?: QueryStringParameterKey[]
defaultQuerystringParameters?: QueryStringParameters
defaultParameters?: QueryStringParameters
}
export class StateActivateContext extends EventEmitter {
domApi: DomApi
data: StateData
parameters: QueryStringParameters
content: StateResolveContent
}
type StateData = {
[name: string]: any
}
type StateTemplate = any
type StateResolveContent = any
type QueryStringParameterKey = string
type QueryStringParameterValue = string | number | null
export interface QueryStringParameters {
[QueryStringParameterKey: string]: QueryStringParameterValue
}
type Falsy = false | 0 | "" | null | undefined
}
Also, because it was mentioned, below is my version of svelte-state-renderer.d.ts
. This requires a svelte.d.ts
file too, which is at the bottom.
Note: My setup doesn't have typescript support within the svelte templates. I'm not sure if OP's method does? These files allow me to use typescript in the files that set my routes, and then within the resolve functions.
PS: I copied a bunch of these files from somewhere.
declare module "svelte-state-renderer" {
import { IMakeRenderer } from "abstract-state-router"
interface DefaultOptions {
data?: any
props?: any
}
export default function SvelteStateRendererFactory(
defaultOptions?: DefaultOptions
): IMakeRenderer
}
svelte.d.ts
declare module "*.svelte" {
export default Svelte
}
declare class Svelte {
constructor(options: { target: Element; data?: any })
get(name?: string): any
set(data: any): void
on(
eventName: string,
callback?: (event?: any) => any
): () => { cancel: () => any }
fire(eventName: string, event?: any)
observe(
name: string,
callback: (newValue?, oldValue?) => any,
options?: { init?: boolean; defer?: boolean }
): () => { cancel: () => any }
oncreate(target): void
ondestroy(): void
destroy(): void
}
declare class ISvelte<T> extends Svelte {
get(): T
get(name: string): any
set(data: T): void
}
Thanks for the input @sw-clough! I don't have a TypeScript project using ASR yet, so my peer review here is going to be weak.
@ArtskydJ I don't consider that listeners
change breaking because the ASR event emitter originally used the node events
polyfill supplied by browserify, which doesn't support that second argument to listeners
.
eventemitter3 was brought in as a much smaller implementation of that API.
So, there is probably still some code out there that sets setMaxListeners
to get rid of the warning that would pop up when you had more than 10 listeners, but I wouldn't expect any code to be relying on the second listeners
argument, since the node emitter never had that function.
I've updated with the 2 changes @sw-clough have listed.
Thanks for your review
I also have types for svelte-state-renderer ready, however it depends on @pyoner/svelte-typescript which is probably not yet the community's standard?
Here it is:
The intention is that the return type of
createStateRouter(makeSvelteStateRenderer({}))
is aliased to SvelteRouter so that users don't need to write out the generics every time.