Intersection Observer Meta #696
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.
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.
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.
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.
I see what your saying, feels like a pretty edge case though.
2d1a479 to
c68e86b
Compare
src/meta/Intersection.ts
Outdated
| return details.entries.get(node) || defaultIntersection; | ||
| } | ||
|
|
||
| public has(key: string | number, options?: IntersectionGetOptions): boolean { |
| } | ||
|
|
||
| private _getDetails(options: IntersectionGetOptions = {}): IntersectionDetail | undefined { | ||
| return this._details.get(JSON.stringify(options)); |
There was a problem hiding this comment.
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.
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 { |
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.
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