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

Add typings file #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jonaskello
Copy link

This PR is just to support the discussion in #31.

@jonaskello jonaskello mentioned this pull request Sep 4, 2018
Copy link
Owner

@andrejewski andrejewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍥

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
export interface Program<S, M, V> {
readonly init?: Change<S, M>;
readonly update: Update<M, S>;
readonly view: View<M, S, V>;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with generic V is that for raj/runtime view is <State, Message>(state: State, dispatch: Dispatch<Message>): void as Raj does nothing with the return value. The return value of view is given meaning in the view libraries such as raj-react. Typing this we either over generalize the core framework or have multiple Program definitions that end-users would need to reconcile.

I'm not saying this is wrong, just something to consider.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this only makes sense if we consider raj-react or similar packages. I've already made typings for raj-react so I'll make a similar PR to that repo in order to put the generic V in context for discussing these typings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the typings for raj-react to put theV in context are here. Not sure if it is really needed, maybe it could be done somehow without V.

@mindplay-dk
Copy link

@jonaskello is there any particular reason why the type-arguments occur in inconsistent order? I'd suggest making them consistent with Program, e.g. S, M, V - specifically:

View<S,M,V>
Update<S,M>

I'd also personally prefer names like TState, TMessage, TView as single letters are not very semantic and the T prefix is idiomatic to Typescript and helps you visually spot the type-variables when reading over the type-declarations.

By the way, Raj made no sense to me until I saw these type-declarations 😉

@jonaskello
Copy link
Author

Yes, I agree we should make the order of type params consistent if this would ever get merged.

Regarding TView, having done a lot of C# that is what I'm most used to too. However I've seen the typescript community use single letters a lot (for example, the official typings for both redux and react have single letters IIRC). I've found that having the longer names can sometimes make reading the typings more difficult since it becomes a bit verbose, especially when the params are repeated many times on the same line. With longer names, each type parameter become more clear but the the line as a whole becomes harder to parse. In this particular case I would say that the meaning of the letters is quite obvious so I would probably stick with single letters, but I have no strong opinion on this.

Btw, I implemented my own version of raj in my project and made a raj-logger as a fork of redux-logger and it is working out quite well. The downside to having no types in raj is that it is hard to make community packages for it like raj-logger since that package would have to import the raj package typings, which are non-existent at this point.

If anyone is familiar with the process of gettings types published as @types packages they are welcome to take these typings and publish them as @types/raj. This might be the best way to go as it seems the raj author is reluctant to include the typings file directly in the package.

@mindplay-dk
Copy link

mindplay-dk commented Oct 18, 2018

Just my opinion, but I'm okay with T as a convention for a single, unconstrained type - and there's a lot of that going around in the community and in Typescript's built-in types. Usually the meaning of the type-argument T is completely plain, e.g. Box<T>, where T is simply "the type". Once you introduce several single letters, their meaning becomes more succint - there's a bit of guesswork involved when you first lay eyes on the thing, and the single letters are much harder for me to parse than actual words.

For example, I instantly thought M was for "model", which is common, and took me a minute to realize it's "message" in this architecture - that was confusing.

@mindplay-dk
Copy link

@jonaskello did you give up on this? (or did it land somewhere else?)

@andrejewski I wish you were more receptive to this. I like the idea of Raj, but I'm basically "on hold" waiting for proper type-definitions - this is likely a roadblock to anyone using TS, since adding correct types for this isn't trivial. Investing in this currently seems too risky for a large-scale project.

Just an aside, but many IDEs by now exploit type-declarations well enough to provide better validation, auto-completion and navigation, even in plain JS projects - so this benefits not only TS users.

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.

3 participants