Treora commented on pull request #87:
URL:
https://github.com/apache/incubator-annotator/pull/87#issuecomment-679141635
> Make several changes to eliminate unnecessary type definitions.
Are we talking about “unnecessary” in a pedagogical or technical sense? :)
(my view has been that types are primarily there to help readers create an
mental overview of the concepts that we deal with in the code base;
implementation needs come second)
> Remove the centrally-exported selector interfaces from the selector
> package and migrate them to where they are used. The selector package
> need not commit to the existence of any particular set of selectors. In
> the future, the apache-annotator package may consolidate the existing
> selector definitions and export them under a unified namespace.
:question: My thinking was rather that the type definitions would be based
on the WA specification, rather than on the needs of our implementation. The
type definitions could be considered a product by itself, in theory even usable
without using any of our implementations. But we could also decide that this is
not a goal of ours.
> Remove the matcher interface in favor of being explicit and verbose about
function types.
:+1: If we use the same signature throughout, and as part of our user-facing
API, give the signature a name seems helpful; on the other hand, if that hides
its definition it might also be obscuring things. With our current small API
either way seems fine to me, so we could just remove them now and see if we
feel a need to reintroduce such names at some point.
> Simplify the type signature for makeRefinable. The selector is defined
> as an interface, thus callers are free to pass anything that extends it,
> so there is no need to parametrize its type. …
:-1: But callers are not free to pass any type of `Selector`. When
assigning `f = makeRefinable(createTextQuoteSelectorMatcher)`, `f`’s parameter
must still be a `TextQuoteSelector`, not just any `Selector`.
In a quick test, after this change even
`makeRefinable(createTextQuoteSelectorMatcher)` seems to be invalid because of
mismatching types, so this needs more scrutiny anyway. (perhaps we could make
some integration tests, and/or turn the demo into typescript, to discover such
errors more easily)
> … Use the same type definition for the input and output of the selector
creator.
:+1:
> Define the DOM match functions in terms of Range and place the burden on
> the caller to pass a Range. Remove the unused scope functions. This
> change may hurt the developer experience, but that will require some
> experience and feedback.
:+1: Hurting developer experience sounds undesirable; but this change makes
the code and API simpler, which may actually help developers. For ease of use,
we could consider providing a simple helper to turn a nodes into Range:
```
export function nodeToRange(node: Node): Range {
const range = (node.ownerDocument ?? node as Document).createRange();
range.selectNodeContents(node);
return range;
}
```
> Remove the "type" field from all selector definitions, since the match
> functions do not use this information. Applications may decide to
> include type information in selectors, but it is not necessary to
> dictate the specific way to achieve this. In the future, utility
> functions in the selector package may dictate this by providing a typed
> selector creator that delegates to implementations based on a type
> field.
:question: Indeed technically we do not need the `type` field, but (in case
we take a spec-first focus) that seems an implementation detail. On the other
hand, it’s also a bit silly to require adding the `type` everywhere (e.g. in
our test cases) just for spec conformance. As a compromise, we could also
consider defining the types according to spec, and then creating supertypes
using `Pick<…, …>` or `Omit<…, 'type'>` in our implementation.
I’m fine with merging the proposed changes (after fixing the refinement
issue) and then reconsidering some aspects later once we better know what we’d
want. I suppose the whole distinction between selector and dom package, and
what exactly they export, will only make sense once we have tools for non-dom
environments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
