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

Make return type of ResolvedPos.node optional #24

Closed
wants to merge 1 commit into from

Conversation

torifat
Copy link

@torifat torifat commented Apr 17, 2018

ResolvedPos.node can actually return undefined for wrong depth.

`ResolvedPos.node` can actually return `undefined` for wrong `depth`.
@marijnh
Copy link
Member

marijnh commented Apr 17, 2018

I consider that an invalid use of the method, not something that should be reflected in its type.

@marijnh marijnh closed this Apr 17, 2018
@torifat torifat deleted the patch-1 branch April 17, 2018 05:56
@d4rkr00t
Copy link
Contributor

I consider that an invalid use of the method, not something that should be reflected in its type.

IMHO from the type definition/documentation of the method it's not clear that you can't pass some numbers it just says number -> Node which is not true :( Because it's actually number -> Node | undefined. And TBH I don't think there is such a thing as "invalid use of method that shouldn't be reflected in its type" because types are either correct e.g. represent what actually happens or they are not...

Also I'm not sure if it's possible to clarify that in documentation somehow, because undefined can appear in two cases:

  1. depth < 0
  2. depth > path.length

@bradleyayers
Copy link
Contributor

This is an interesting scenario, where I think it ultimately boils down to a mismatch of goals for the docs. The two goals I see for the docs:

  1. Provide a human-friendly description of the type that's less ambiguous than English.
  2. Serve as an input for generating TypeScript declarations to provide static type checking verification.

Obviously from a static-type safety perspective, having accurate types is more important, but I can see from a user-documentation perspective keeping the types "clean" is desirable.

One approach forward would be to maintain a set of "type overrides" in the tool that generates the TypeScript declarations, but I'd like to avoid that. However if the goals really are irreconcilable then forking the types probably does make the most sense.

Another idea I've been thinking about is having a separate traversal library (think jQuery but for ProseMirror) that would expose an API that has type safety as a goal. This would decouple the goals of ProseMirror from those of consumers who value static type safety.

@marijnh
Copy link
Member

marijnh commented Apr 17, 2018

In this case I think it's preferable for both the docs and the typescript types to have a non-optional return type here. I do not want to force users of the code to insert a null check when they are using the function correctly.

There's only a certain amount of information you can encode in types—unless you are programming in Coq or Idris, you're going to be able to write invalid code that still satisfies the type checker.

@bradleyayers
Copy link
Contributor

FWIW there's definitely precedence for that principle (array indexing being non-nullable) with people on both sides: microsoft/TypeScript#11122 (comment)

Thanks for elaborating on the design decision!

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.

4 participants