range.startContainer is undefined

[ziodave]

It looks that this change [1] is raising an exception that range.startContainer is undefined:

Because range is not a Range but an Element.

This is the call stack:

This is my test code:

async function highlightMatcher(matcher: any) {
  const matches = matcher(
    // @ts-ignore
    document.body
  );
  for await (const match of matches) {
    highlightRange(match);
  }
}

highlightMatcher(
  createCssSelectorMatcher({
    value: "#my-div",
    type: "CssSelector",
  })
);

[1] https://github.com/apache/incubator-annotator/commit/7f9461da824297947196a34ac5c3af7e52e2caf7#diff-1a23dff7a2e2636f389f030fcf64a2a1f6b4f8c278a0c2b231a910906b4454a3R67

[ziodave]

If I revert https://github.com/apache/incubator-annotator/commit/7f9461da824297947196a34ac5c3af7e52e2caf7#diff-1a23dff7a2e2636f389f030fcf64a2a1f6b4f8c278a0c2b231a910906b4454a3R67 to return a Range it works.

[Treora]

Hi David, this indeed happens because we recently changed the API for CssSelectors. My reasoning was that the target of a CssSelector will always be an Element (or multiple), so returning that element may be more natural, and more practical especially for people that only use the CssSelector and not others.

On the other hand, it makes that not every matcher function returns the same type of match. The change went together with making every matcher function accept either a Range or an Element for their scope argument (instead of just a Range), so that you can still e.g. refine a CssSelector with a TextQuoteSelector (i.e. the CssSelector’s matching element(s) is the scope within which the quote will be searched). You make me realise that we should then also change highlightRange to accept an Element too (and rename it accordingly).

Your thoughts on these API changes are very welcome; we need more user feedback to learn what works for people. Perhaps we could make more breaking changes to elicit such responses. ;)

[Treora]

I renamed highlightRange to highlightText (seems a better name in any case!), and made it accept either a Node or Range (internally we just use a function toRange to convert it to a range, easy enough). Your example code should work now, if you pull the latest commit and update the import to reflect the renaming.

By the way, you could let TypeScript help you find errors like this by giving the matcher argument a type (instead of any). In your case it could be Matcher<Element, Element | Range>: the TScope type parameter only needs to be Element, because you always pass it document.body, and all Matcher functions in the dom package yield either Ranges or Elements. Or, if you want to make it more generic, I think the type could be Matcher<Node | Range, Node | Range>: you can pass any Node or Range to any of the matchers, and every Element is a Node so if you like symmetry the TMatch type parameter could also be made Node | Range (as highlightText now accepts this too).

[ziodave]

Your thoughts on these API changes are very welcome; we need more user feedback to learn what works for people. Perhaps we could make more breaking changes to elicit such responses. ;)

I am trying to build a prototype using this project, I'll share my feedback as much as possible of course.

Do you also have on- or off-line events, community chat or similar where we can get to know each other and share more knowledge about the project and how to use it?

[Treora]

Do you also have on- or off-line events, community chat or similar where we can get to know each other and share more knowledge about the project and how to use it?

Apologies for the lack of response to this. You are more than welcome to join our IRC channel and mailing list (details on our homepage, and join our community call (usually Thursdays 15:00 UTC on jitsi — but best to always check on IRC if people are available that week). Or just reach out to have a chat anytime.