| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
src/meta/Intersection.ts
Outdated
|
|
||
| private _createDetails(options: IntersectionGetOptions, rootNode?: HTMLElement): IntersectionDetail { | ||
| const entries = new WeakMap<HTMLElement, ExtendedIntersectionObserverEntry>(); | ||
| const observer = new global.IntersectionObserver(this._onIntersect(entries), options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root string from the options object (instead of the root node) is passed when creating an IntersectionObserver. I have a test for this in: 'intersections accept root'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense, thanks (i'll add a test).
I actually tried the root node functionality yesterday but couldn't get it to work (just thought I had done it wrong.)
src/meta/Intersection.ts
Outdated
| const observer = new global.IntersectionObserver(this._onIntersect(entries), options); | ||
| const details = { observer, entries, ...options }; | ||
|
|
||
| this._details.set(JSON.stringify(options), details); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I was wondering about this when I was working on it. If the key order changes, will this encode the same thing? Is it a big deal if it doesn't? I had gone with the approach of using JSON.stringify([root, rootMargin, thresholds]) to make sure the structure stays the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what your saying, feels like a pretty edge case though.
agubler
force-pushed
the
feature-meta-intersection
branch
from
2d1a479 to
c68e86b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments/thoughts
src/meta/Intersection.ts
Outdated
| return details.entries.get(node) || defaultIntersection; | ||
| } | ||
|
|
||
| public has(key: string | number, options?: IntersectionGetOptions): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some JSDOC would be nice
| } | ||
|
|
||
| private _getDetails(options: IntersectionGetOptions = {}): IntersectionDetail | undefined { | ||
| return this._details.get(JSON.stringify(options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit concerned about GC with this... I can understand why stringifying is useful to create a key here, but it is a map, which means it will have strong references to the values, which means that they won't ever get released. Shouldn't here be a destruction handle in the constructor to .clear() the Map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use Maps across the widget-core, should we raise an separate issue to discuss this?
src/meta/Intersection.ts
Outdated
| export class Intersection extends Base { | ||
| private readonly _details: Map<string, IntersectionDetail> = new Map(); | ||
|
|
||
| public get(key: string, options: IntersectionGetOptions = {}): IntersectionResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some JSDOC would be nice
src/meta/Intersection.ts
Outdated
| }); | ||
|
|
||
| export class Intersection extends Base { | ||
| private readonly _details: Map<string, IntersectionDetail> = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, but you could private readonly _details = new Map<string, IntersectionDetail>();
Type: feature
The following has been addressed in the PR:
Description:
Builds on #664, updating to use the latest changes in meta (added by by #666).
It is highly likely the functionality will need to be enhanced to cater for the more advanced use-cases in the future.
Includes the intersection observer polyfil
Resolves #644