Skip to content

[6.0] Fix loading performance for pages with many links caused by excessive re-computation #912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
});
});