Remove cyclic references in RemoteDocumentCache #3857
Conversation
|
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis ReportAffected ProductsNo changes between base commit (d4db75f) and head commit (446954f). Test Logs
|
| * The RemoteDocumentCache for IndexedDb. To construct, invoke | ||
| * `newIndexedDbRemoteDocumentCache()`. | ||
| */ | ||
| export class IndexedDbRemoteDocumentCacheImpl implements RemoteDocumentCache { |
There was a problem hiding this comment.
Can "export" be removed from the declaration of IndexedDbRemoteDocumentCacheImpl? It doesn't appear to be used outside of this file and its name implies that it is an implementation detail that should not be exposed.
There was a problem hiding this comment.
The entire idea behind the XYZ and XYZImpl is that XYZImpl is not exported, so that the public fields in XYZImpl are only visible within the module. So, yes, this should not be exported.
| * The RemoteDocumentCache for IndexedDb. To construct, invoke | ||
| * `newIndexedDbRemoteDocumentCache()`. | ||
| */ | ||
| export class IndexedDbRemoteDocumentCacheImpl implements RemoteDocumentCache { |
There was a problem hiding this comment.
Should IndexedDbRemoteDocumentCache implement IndexedDbRemoteDocumentCache instead of RemoteDocumentCache?
There was a problem hiding this comment.
Yes. Thanks for catching.
| * The memory-only RemoteDocumentCache for IndexedDb. To construct, invoke | ||
| * `newMemoryRemoteDocumentCache()`. | ||
| */ | ||
| class MemoryRemoteDocumentCacheImpl implements RemoteDocumentCache { |
There was a problem hiding this comment.
Should MemoryRemoteDocumentCacheImpl implement MemoryRemoteDocumentCache instead of RemoteDocumentCache?
There was a problem hiding this comment.
Yes. Thanks for catching.
| indexManager: IndexManager, | ||
| sizer: DocumentSizer | ||
| ): MemoryRemoteDocumentCache { | ||
| return new MemoryRemoteDocumentCacheImpl(indexManager, sizer); |
There was a problem hiding this comment.
TypeScript noobie question: How does this compile since MemoryRemoteDocumentCacheImpl does not implement MemoryRemoteDocumentCache?
There was a problem hiding this comment.
TypeScript interfaces are enforced by duck-typing - if a type happens to implement all members of an interface, it is considered identical to a type that explicitly implements the interface. Explicitly implementing comes in handy when the methods are renamed or added as it will produce much saner errors.
This PR removes a static inheritance that WebPack cannot tree-shake.
Before, our RemoteDocumentCaches would look like this:
This patterns breaks code optimization in Webpack (as per @Feiyang1's research), but it does allow us to access "private" members within the enclosing class while also extending from another class. To address the Webpack issue while keeping some of the encapsulation, I:
The rest of the repo still refers to the old types, which are now interfaces and hide the new public methods. This is the same pattern that I applied in lots of other places already (see LocalStore), and it also allowed me to make two functions tree-shakeable that are only used in Multi-Tab (
getNewDocumentChanges()andgetReadTime()).There are no logic changes.