Skip to content

Commit

Permalink
Fix loading performance for pages with many links caused by excessive…
Browse files Browse the repository at this point in the history
… re-computation (#900)

Resolves: rdar://136247329
  • Loading branch information
mportiz08 authored Oct 9, 2024
1 parent bc2d4bc commit f7c7899
Show file tree
Hide file tree
Showing 10 changed files with 167 additions and 86 deletions.
6 changes: 6 additions & 0 deletions src/components/DocumentationTopic.vue
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ export default {
return {
reset() {},
state: {},
setReferences() {},
updateReferences() {},
};
},
},
Expand Down Expand Up @@ -423,6 +425,7 @@ export default {
},
data() {
return {
appState: AppStore.state,
topicState: this.store.state,
declListExpanded: false, // Hide all other declarations by default
};
Expand Down Expand Up @@ -718,6 +721,9 @@ export default {
this.store.setReferences(this.references);
},
watch: {
'appState.includedArchiveIdentifiers': function updateRefs() {
this.store.updateReferences();
},
// update the references in the store, in case they update, but the component is not re-created
references(references) {
this.store.setReferences(references);
Expand Down
44 changes: 1 addition & 43 deletions src/mixins/referencesProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
* See https://swift.org/LICENSE.txt for license information
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/
import AppStore from 'docc-render/stores/AppStore';

const TopicReferenceTypes = new Set([
'section',
'topic',
]);

export default {
// inject the `store`
Expand All @@ -27,43 +21,7 @@ export default {
}),
},
},
data: () => ({ appState: AppStore.state }),
computed: {
// exposes the references for the current page
references() {
const {
isFromIncludedArchive,
store: {
state: { references: originalRefs = {} },
},
} = this;
// strip the `url` key from "topic"/"section" refs if their identifier
// comes from an archive that hasn't been included by DocC
return Object.keys(originalRefs).reduce((newRefs, id) => {
const originalRef = originalRefs[id];
const { url, ...refWithoutUrl } = originalRef;
return TopicReferenceTypes.has(originalRef.type) ? ({
...newRefs,
[id]: isFromIncludedArchive(id) ? originalRefs[id] : refWithoutUrl,
}) : ({
...newRefs,
[id]: originalRef,
});
}, {});
},
},
methods: {
isFromIncludedArchive(id) {
const { includedArchiveIdentifiers = [] } = this.appState;
// for backwards compatibility purposes, treat all references as being
// from included archives if there is no data for it
if (!includedArchiveIdentifiers.length) {
return true;
}

return includedArchiveIdentifiers.some(archiveId => (
id?.startsWith(`doc://${archiveId}/`)
));
},
references: ({ store }) => store.state.references,
},
};
6 changes: 5 additions & 1 deletion src/stores/DocumentationTopicStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import { filterInactiveReferences } from 'docc-render/utils/references';
import ApiChangesStoreBase from 'docc-render/stores/ApiChangesStoreBase';
import OnThisPageSectionsStoreBase from 'docc-render/stores/OnThisPageSectionsStoreBase';
import Settings from 'docc-render/utils/settings';
Expand Down Expand Up @@ -36,7 +37,10 @@ export default {
this.state.contentWidth = width;
},
setReferences(references) {
this.state.references = references;
this.state.references = filterInactiveReferences(references);
},
updateReferences() {
this.setReferences(this.state.references);
},
...changesActions,
...pageSectionsActions,
Expand Down
47 changes: 47 additions & 0 deletions src/utils/references.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* This source file is part of the Swift.org open source project
*
* Copyright (c) 2024 Apple Inc. and the Swift project authors
* Licensed under Apache License v2.0 with Runtime Library Exception
*
* See https://swift.org/LICENSE.txt for license information
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import AppStore from 'docc-render/stores/AppStore';

const TopicReferenceTypes = new Set([
'section',
'topic',
]);

function isFromIncludedArchive(id) {
const { includedArchiveIdentifiers } = AppStore.state;

// for backwards compatibility purposes, treat all references as being
// from included archives if there is no data for it
if (!includedArchiveIdentifiers.length) {
return true;
}

return includedArchiveIdentifiers.some(archiveId => (
id?.startsWith(`doc://${archiveId}/`)
));
}

function filterInactiveReferences(references = {}) {
return Object.keys(references).reduce((newRefs, id) => {
const originalRef = references[id];
const { url, ...refWithoutUrl } = originalRef;
return TopicReferenceTypes.has(originalRef.type) ? ({
...newRefs,
[id]: isFromIncludedArchive(id) ? originalRef : refWithoutUrl,
}) : ({
...newRefs,
[id]: originalRef,
});
}, {});
}

// eslint-disable-next-line import/prefer-default-export
export { filterInactiveReferences };
20 changes: 20 additions & 0 deletions tests/unit/components/DocumentationTopic.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,26 @@ describe('DocumentationTopic', () => {
expect(mockStore.setReferences).toHaveBeenCalledWith(newReferences);
});

it('calls `store.updateReferences` when `appState.includedArchiveIdentifiers` changes', async () => {
const store = {
state: { references: {} },
reset: jest.fn(),
setReferences: jest.fn(),
updateReferences: jest.fn(),
};
wrapper = shallowMount(DocumentationTopic, {
propsData,
provide: { store },
});
expect(store.updateReferences).not.toHaveBeenCalled();

wrapper.setData({
appState: { includedArchiveIdentifiers: ['Foo', 'Bar'] },
});
await wrapper.vm.$nextTick();
expect(store.updateReferences).toHaveBeenCalled();
});

describe('lifecycle hooks', () => {
it('calls `store.reset()`', () => {
jest.clearAllMocks();
Expand Down
39 changes: 0 additions & 39 deletions tests/unit/mixins/referencesProvider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,43 +91,4 @@ describe('referencesProvider', () => {
expect(inner.exists()).toBe(true);
expect(inner.props('references')).toEqual(references);
});

it('removes `url` data for refs with non-empty `includedArchiveIdentifiers` app state', () => {
// empty `includedArchiveIdentifiers` — no changes to refs
const outer = createOuter();
let inner = outer.find(FakeComponentInner);
expect(inner.exists()).toBe(true);
expect(inner.props('references')).toEqual(references);

// `includedArchiveIdentifiers` contains all refs - no changes to refs
outer.setData({
appState: {
includedArchiveIdentifiers: ['A', 'B', 'BB'],
},
});
inner = outer.find(FakeComponentInner);
expect(inner.exists()).toBe(true);
expect(inner.props('references')).toEqual(references);

// `includedArchiveIdentifiers` only contains archive B — remove `url` field
// from all non-B refs
outer.setData({
appState: {
includedArchiveIdentifiers: ['B'],
},
});
inner = outer.find(FakeComponentInner);
expect(inner.exists()).toBe(true);
const refs3 = inner.props('references');
expect(refs3).not.toEqual(references);
expect(refs3[aa.identifier].title).toBe(aa.title);
expect(refs3[aa.identifier].url).toBeFalsy(); // aa `url` is gone now
expect(refs3[ab.identifier].title).toBe(ab.title);
expect(refs3[ab.identifier].url).toBeFalsy(); // ab `url` is gone now
expect(refs3[bb.identifier].title).toBe(bb.title);
expect(refs3[bb.identifier].url).toBe(bb.url); // bb still has `url`
expect(refs3[bbb.identifier].title).toBe(bbb.title);
expect(refs3[bbb.identifier].url).toBeFalsy(); // bbb `url` is gone now
expect(refs3[c.identifier].url).toBe(c.url); // external link untouched
});
});
11 changes: 10 additions & 1 deletion tests/unit/stores/DocumentationTopicStore.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import { filterInactiveReferences } from 'docc-render/utils/references';
import DocumentationTopicStore from 'docc-render/stores/DocumentationTopicStore';
import ApiChangesStoreBase from 'docc-render/stores/ApiChangesStoreBase';
import OnThisPageSectionsStoreBase from 'docc-render/stores/OnThisPageSectionsStoreBase';
Expand Down Expand Up @@ -101,7 +102,15 @@ describe('DocumentationTopicStore', () => {

it('sets `references`', () => {
DocumentationTopicStore.setReferences(references);
expect(DocumentationTopicStore.state.references).toBe(references);
expect(DocumentationTopicStore.state.references)
.toEqual(filterInactiveReferences(references));
});

it('updates `references`', () => {
const prevState = DocumentationTopicStore.state;
DocumentationTopicStore.updateReferences();
expect(DocumentationTopicStore.state.references)
.toEqual(filterInactiveReferences(prevState.references));
});

describe('APIChanges', () => {
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/stores/TopicStore.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ describe('TopicStore', () => {
describe('setReferences', () => {
it('sets the `references` state', () => {
TopicStore.setReferences(references);
expect(TopicStore.state.references).toBe(references);
expect(TopicStore.state.references).toEqual(references);
});
});
});
2 changes: 1 addition & 1 deletion tests/unit/stores/TutorialsOverviewStore.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('TutorialsOverviewStore', () => {
describe('setReferences', () => {
it('sets the `references` state', () => {
TutorialsOverviewStore.setReferences(references);
expect(TutorialsOverviewStore.state.references).toBe(references);
expect(TutorialsOverviewStore.state.references).toEqual(references);
});
});
});
76 changes: 76 additions & 0 deletions tests/unit/utils/references.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* This source file is part of the Swift.org open source project
*
* Copyright (c) 2024 Apple Inc. and the Swift project authors
* Licensed under Apache License v2.0 with Runtime Library Exception
*
* See https://swift.org/LICENSE.txt for license information
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import AppStore from 'docc-render/stores/AppStore';
import { filterInactiveReferences } from 'docc-render/utils/references';

const aa = {
identifier: 'doc://A/documentation/A/a',
url: '/documentation/A/a',
title: 'A.A',
type: 'topic',
};
const ab = {
identifier: 'doc://A/documentation/A/b',
url: '/documentation/A/b',
title: 'A.B',
type: 'topic',
};
const bb = {
identifier: 'doc://B/documentation/B/b',
url: '/documentation/B/b',
title: 'B.B',
type: 'topic',
};
const bbb = {
identifier: 'doc://BB/documentation/BB/b',
url: '/documentation/BB/b#b',
title: 'BB.B',
type: 'section',
};
const c = {
identifier: 'https://abc.dev',
url: 'https://abc.dev',
title: 'C',
type: 'link',
};

const references = {
[aa.identifier]: aa,
[ab.identifier]: ab,
[bb.identifier]: bb,
[bbb.identifier]: bbb,
[c.identifier]: c,
};

describe('filterInactiveReferences', () => {
it('does not filter any refs when `includedArchiveIdentifiers` is empty', () => {
AppStore.setIncludedArchiveIdentifiers([]);
expect(filterInactiveReferences(references)).toEqual(references);
});

it('does not filter any refs when `includedArchiveIdentifiers` includes all ref archives', () => {
AppStore.setIncludedArchiveIdentifiers(['A', 'B', 'BB']);
expect(filterInactiveReferences(references)).toEqual(references);
});

it('removes `url` from non-external refs that aren\'t part of included archive', () => {
AppStore.setIncludedArchiveIdentifiers(['B']);
const filteredRefs = filterInactiveReferences(references);

expect(Object.keys(filteredRefs)).toEqual(Object.keys(references));

expect(filteredRefs[aa.identifier].url).toBeFalsy();
expect(filteredRefs[ab.identifier].url).toBeFalsy();
expect(filteredRefs[bb.identifier].url).toBe(bb.url);
expect(filteredRefs[bbb.identifier].url).toBeFalsy();
expect(filteredRefs[c.identifier].url).toBe(c.url);
});
});

0 comments on commit f7c7899

Please sign in to comment.