You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@annotator.apache.org by GitBox <gi...@apache.org> on 2020/09/07 23:24:00 UTC
[GitHub] [incubator-annotator] tilgovi opened a new pull request #88: Restrict DOM scopes to be instances of Range
tilgovi opened a new pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88
Define the DOM match functions in terms of Range and place the burden on
the caller to pass a Range. Remove the scope helper functions.
This change simplifies the code at the possible cost of developer
experience, but understanding the tradeoff will require some experience
and feedback.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689017845
So, to summarize, I'd rather you left the inline `ownerDocument` code, but I'm okay with this as a temporary change before we extract a package. I like the lint rule, because I would like to encourage us to make packages (that's why we have a monorepo), but I'm okay with it temporarily.
I appreciate the other changes and think this is good. I'll approve, but recommend renaming `util.ts` to just `owner-document.ts` or `scope.ts`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689010515
> I noticed that the ownerDocument(range) helper was not permitted by the eslint rule forbidding relative parent imports. Was that your reason to repeat the exact same lines in several files?
No, that wasn't the reason. I felt that it was a couple lines of simple destructing and one nullish coalescing assignment. In context, it might often be useful to destructure other properties of the range at the same time. I'm fine to have a helper function, but it didn't feel necessary or very clarifying.
I still do want to look at changing the lint rules, I'm sorry I have not had time to yet.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r485064755
##########
File path: packages/dom/src/text-quote/describe.ts
##########
@@ -21,44 +21,31 @@
import seek from 'dom-seek';
import type { TextQuoteSelector } from '@annotator/selector';
-import type { DomScope } from '../types';
-import { ownerDocument, rangeFromScope } from '../scope';
-
export async function describeTextQuote(
range: Range,
- scope: DomScope = ownerDocument(range).documentElement,
+ scope: Range = range,
Review comment:
I changed it so that it took a range, because it simplified things and made this symmetric with the anchoring.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
Treora commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689082728
> I'll approve, but recommend renaming `util.ts` to just `owner-document.ts` or `scope.ts`.
As it is not specific to scopes and now also used for other ranges, `owner-document.ts` seems better.
> I felt that it was a couple lines of simple destructing and one nullish coalescing assignment. In context, it might often be useful to destructure other properties of the range at the same time. I'm fine to have a helper function, but it didn't feel necessary or very clarifying.
Ok. I think we have different priorities; for me, even if it is just two or three lines of code, the fact that the *intention* of the code is somewhat obscured makes it an undesirable distraction for a reader. I suppose it deserves a comment line to explain the need for the type assertion, so I’ll add that while at it. Moreover I don’t see the usefulness of destructuring in these cases, as it results in variable names that hide their context (“whose ‘commonAncestorContainer’ are we talking about, that of scope or of the other range?”). It may be clear by now that I write code primarily for people, not computers. :)
I like folders, but I’m okay with flatter packages and/or more packages. As long as it is easy to find out which things belong together.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689016825
This looks good. I think I'm okay with removing the relative import rule for now. I don't like having modules called `util`, or packages called `common` or `utils` or anything like this, much, though. I would prefer to leave it in a file called `scope.ts`.
I might wish to flatten everything a bit. I understand you like grouping relating things, but I don't mind longer files for groupings or filename prefixes. We have a monorepo so that we can make many packages easily. For any of those packages to have much internal hierarchy or structure feels wrong to me. Having the parent relative imports feels like we haven't given things the right structure. We should be able to easily make a package, like `@annotator/dom-range` to have range related utilities, or simply `@annotator/dom` to have DOM-related utilities. For this reason, I think I would prefer to just inline the two lines of `ownerDocument()` and then do that separately.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r485068658
##########
File path: packages/dom/src/text-quote/describe.ts
##########
@@ -21,44 +21,31 @@
import seek from 'dom-seek';
import type { TextQuoteSelector } from '@annotator/selector';
-import type { DomScope } from '../types';
-import { ownerDocument, rangeFromScope } from '../scope';
-
export async function describeTextQuote(
range: Range,
- scope: DomScope = ownerDocument(range).documentElement,
+ scope: Range = range,
Review comment:
Oh, my mistake, I am thinking of the other function to calculate context.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r484861032
##########
File path: packages/dom/src/highlight-range.ts
##########
@@ -73,8 +73,9 @@ function textNodesInRange(range: Range): Text[] {
}
// Collect the text nodes.
- const document =
- range.startContainer.ownerDocument || (range.startContainer as Document);
+ const { commonAncestorContainer } = range;
+ const { ownerDocument } = commonAncestorContainer;
+ const document = ownerDocument ?? (commonAncestorContainer as Document);
Review comment:
Any reason for this change?
In several places, I have so far tried to only use the properties of [AbstractRange](https://developer.mozilla.org/en-US/docs/Web/API/AbstractRange) when possible, to see if we can let users pass anything with this minimal (5 attribute) interface instead an actual Range. Do you think it may be worth pursuing? If we go this way we could change our types throughout, and add a simple helper function `scopeToRange` again to ‘hydrate’ our abstract ranges at the start of our functions. :)
##########
File path: packages/dom/src/text-quote/describe.ts
##########
@@ -21,44 +21,31 @@
import seek from 'dom-seek';
import type { TextQuoteSelector } from '@annotator/selector';
-import type { DomScope } from '../types';
-import { ownerDocument, rangeFromScope } from '../scope';
-
export async function describeTextQuote(
range: Range,
- scope: DomScope = ownerDocument(range).documentElement,
+ scope: Range = range,
Review comment:
Should be the document element, not the range itself!
##########
File path: packages/dom/src/range/match.ts
##########
@@ -18,22 +18,21 @@
* under the License.
*/
-import type { RangeSelector, Selector } from '@annotator/selector';
-
-import { ownerDocument } from '../scope';
-import type { DomMatcher, DomScope } from '../types';
Review comment:
If we stop using them, we’d better remove the type file, and the scope file too.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r484880916
##########
File path: packages/dom/src/highlight-range.ts
##########
@@ -73,8 +73,9 @@ function textNodesInRange(range: Range): Text[] {
}
// Collect the text nodes.
- const document =
- range.startContainer.ownerDocument || (range.startContainer as Document);
+ const { commonAncestorContainer } = range;
+ const { ownerDocument } = commonAncestorContainer;
+ const document = ownerDocument ?? (commonAncestorContainer as Document);
Review comment:
Also, these exact lines appear in several places; I’d prefer to just keep the `ownerDocument` helper to hide this implementation detail.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi merged pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi merged pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689018248
Hah, I can't approve my own PR, but I may just merge this in the evening then, with your changes. Thanks for taking a look.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
Treora commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r484884585
##########
File path: packages/dom/src/highlight-range.ts
##########
@@ -73,8 +73,9 @@ function textNodesInRange(range: Range): Text[] {
}
// Collect the text nodes.
- const document =
- range.startContainer.ownerDocument || (range.startContainer as Document);
+ const { commonAncestorContainer } = range;
+ const { ownerDocument } = commonAncestorContainer;
+ const document = ownerDocument ?? (commonAncestorContainer as Document);
Review comment:
> I have so far tried to only use the properties of [AbstractRange](https://developer.mozilla.org/en-US/docs/Web/API/AbstractRange) when possible
Ah, I see I did not apply this in the `ownerDocument` function; I guess you took the approach from there.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] Treora commented on pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
Treora commented on pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-688882247
I hope you don’t mind I turned my suggestions (except using AbstractRange) into commits. I noticed that the `ownerDocument(range)` helper was not permitted by the eslint rule forbidding relative parent imports. Was that your reason to repeat the exact same lines in several files? I suggest we just remove this rule; even though it seems a nice idea, I think it’s really not worth obscuring our code with repeated rote implementation details just for the sake of organisational purity. Or, if you insist, we could publish our helper functions in a package; or (my preference) we’d modify the linter rule to allow relative parent imports from a single ‘utils’ or ‘common’ folder.
It’s not directly related to this PR, but I also fixed two accidental uses of the global `document`, where `ownerDocument(range)` should have been used instead. Extra reason to keep this helper.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-annotator] tilgovi commented on a change in pull request #88: Restrict DOM scopes to be instances of Range
Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r485069411
##########
File path: packages/dom/test/highlight-range/highlight-range.test.ts
##########
@@ -172,7 +172,7 @@ describe('highlightRange', () => {
const inputHtml = `<b>Try highlighting this image: <img> — would that work?</b>`;
const doc = domParser.parseFromString(inputHtml, 'text/html');
- const range = document.createRange();
+ const range = doc.createRange();
Review comment:
Good catch.
----------------------------------------------------------------
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:
users@infra.apache.org