Skip to content

Inactivate known external links #878

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

Merged
Show file tree
Hide file tree
Changes from 7 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
5 changes: 4 additions & 1 deletion src/components/ContentNode/Reference.vue
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export default {
name: 'Reference',
computed: {
isInternal({ url }) {
if (!url) {
return false;
}
if (!url.startsWith('/') && !url.startsWith('#')) {
// If the URL has a scheme, it's not an internal link.
return false;
Expand Down Expand Up @@ -92,7 +95,7 @@ export default {
props: {
url: {
type: String,
required: true,
required: false,
},
kind: {
type: String,
Expand Down
4 changes: 2 additions & 2 deletions src/components/ContentNode/ReferenceExternal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
-->

<template>
<a v-if="isActive" :href="url"><slot /></a>
<a v-if="url && isActive" :href="url"><slot /></a>
<span v-else><slot /></span>
</template>

Expand All @@ -19,7 +19,7 @@ export default {
props: {
url: {
type: String,
required: true,
required: false,
},
isActive: {
type: Boolean,
Expand Down
4 changes: 2 additions & 2 deletions src/components/ContentNode/ReferenceInternal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
-->

<template>
<router-link v-if="isActive" :to="url"><slot /></router-link>
<router-link v-if="url && isActive" :to="url"><slot /></router-link>
<span v-else><slot/></span>
</template>

Expand All @@ -19,7 +19,7 @@ export default {
props: {
url: {
type: String,
required: true,
required: false,
},
isActive: {
type: Boolean,
Expand Down
8 changes: 7 additions & 1 deletion src/components/Navigator/NavigatorDataProvider.vue
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<script>
import { fetchIndexPathsData } from 'docc-render/utils/data';
import { flattenNestedData } from 'docc-render/utils/navigatorData';
import AppStore from 'docc-render/stores/AppStore';
import Language from 'docc-render/constants/Language';
/**
Expand Down Expand Up @@ -88,11 +89,16 @@ export default {
async fetchIndexData() {
try {
this.isFetching = true;
const { interfaceLanguages, references } = await fetchIndexPathsData(
const {
includedArchiveIdentifiers = [],
interfaceLanguages,
references,
} = await fetchIndexPathsData(
{ slug: this.$route.params.locale || '' },
);
this.navigationIndex = Object.freeze(interfaceLanguages);
this.navigationReferences = Object.freeze(references);
AppStore.setIncludedArchiveIdentifiers(includedArchiveIdentifiers);
} catch (e) {
this.errorFetching = true;
} finally {
Expand Down
34 changes: 33 additions & 1 deletion src/mixins/referencesProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* 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';

export default {
// inject the `store`
Expand All @@ -21,8 +22,39 @@ export default {
}),
},
},
data: () => ({ appState: AppStore.state }),
computed: {
// exposes the references for the current page
references: ({ store }) => store.state.references,
references() {
const {
isFromIncludedArchive,
store: {
state: { references: originalRefs = {} },
},
} = this;
// strip the `url` key from refs if their identifier comes from an
// archive that hasn't been included by DocC
return Object.keys(originalRefs).reduce((newRefs, id) => {
const { url, ...refWithoutUrl } = originalRefs[id];
return {
...newRefs,
[id]: isFromIncludedArchive(id) ? originalRefs[id] : refWithoutUrl,
};
}, {});
},
},
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}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing the discussion we started in the original PR: #873 (comment)

I think we need to check if the identifier starts with doc://${id}/documentation. Just to make sure that the id matches exactly. (For example, Foo and FooBar)

That would only work for documentation pages and not tutorial pages. We could make this more strict, but I think checking for the special scheme and bundle ID should be sufficient (@d-ronnqvist correct me if I'm wrong)

You are right, that wouldn't work for tutorial pages. But I still think this wouldn't work if the frameworks in question are named: 'Foo' and 'FooCore'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think we can change this to .startsWith(`doc://${archiveId}/`) instead (just adding the trailing slash) to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test for this edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 6edce2b and included a test case that fails without the trailing slash

));
},
},
};
5 changes: 5 additions & 0 deletions src/stores/AppStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,15 @@ export default {
supportsAutoColorScheme,
systemColorScheme: ColorScheme.light,
availableLocales: [],
includedArchiveIdentifiers: [],
},
reset() {
this.state.imageLoadingStrategy = process.env.VUE_APP_TARGET === 'ide'
? ImageLoadingStrategy.eager : ImageLoadingStrategy.lazy;
this.state.preferredColorScheme = Settings.preferredColorScheme || defaultColorScheme;
this.state.supportsAutoColorScheme = supportsAutoColorScheme;
this.state.systemColorScheme = ColorScheme.light;
this.state.includedArchiveIdentifiers = [];
},
setImageLoadingStrategy(strategy) {
this.state.imageLoadingStrategy = strategy;
Expand All @@ -59,6 +61,9 @@ export default {
setSystemColorScheme(value) {
this.state.systemColorScheme = value;
},
setIncludedArchiveIdentifiers(value) {
this.state.includedArchiveIdentifiers = value;
},
syncPreferredColorScheme() {
if (!!Settings.preferredColorScheme
&& Settings.preferredColorScheme !== this.state.preferredColorScheme) {
Expand Down
19 changes: 19 additions & 0 deletions tests/unit/components/Navigator/NavigatorDataProvider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import NavigatorDataProvider from '@/components/Navigator/NavigatorDataProvider.vue';
import { shallowMount } from '@vue/test-utils';
import AppStore from 'docc-render/stores/AppStore';
import Language from 'docc-render/constants/Language';
import { TopicTypes } from '@/constants/TopicTypes';
import { fetchIndexPathsData } from '@/utils/data';
Expand Down Expand Up @@ -127,7 +128,13 @@ const references = {
foo: { bar: 'bar' },
};

const includedArchiveIdentifiers = [
'foo',
'bar',
];

const response = {
includedArchiveIdentifiers,
interfaceLanguages: {
[Language.swift.key.url]: [
swiftIndexOne,
Expand Down Expand Up @@ -174,6 +181,10 @@ describe('NavigatorDataProvider', () => {
jest.clearAllMocks();
});

afterEach(() => {
AppStore.reset();
});

it('fetches data when mounting NavigatorDataProvider', async () => {
expect(fetchIndexPathsData).toHaveBeenCalledTimes(0);
createWrapper();
Expand Down Expand Up @@ -552,4 +563,12 @@ describe('NavigatorDataProvider', () => {
},
]);
});

it('sets `includedArchiveIdentifiers` state in the app store', async () => {
expect(AppStore.state.includedArchiveIdentifiers).toEqual([]);
fetchIndexPathsData.mockResolvedValue(response);
createWrapper();
await flushPromises();
expect(AppStore.state.includedArchiveIdentifiers).toEqual(includedArchiveIdentifiers);
});
});
113 changes: 113 additions & 0 deletions tests/unit/mixins/referencesProvider.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/**
* 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 { shallowMount } from '@vue/test-utils';
import referencesProvider from 'docc-render/mixins/referencesProvider';

const FakeComponentInner = {
name: 'FakeComponentInner',
props: ['references'],
render() {
return null;
},
};

const FakeComponentOuter = {
name: 'FakeComponentOuter',
mixins: [referencesProvider],
render(createElement) {
return createElement(FakeComponentInner, {
props: {
references: this.references,
},
});
},
};

const aa = {
identifier: 'doc://A/documentation/A/a',
url: '/documentation/A/a',
title: 'A.A',
};
const ab = {
identifier: 'doc://A/documentation/A/b',
url: '/documentation/A/b',
title: 'A.B',
};
const bb = {
identifier: 'doc://B/documentation/B/b',
url: '/documentation/B/b',
title: 'B.B',
};

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

const provide = {
store: {
state: { references },
},
};

const createOuter = (opts = { provide }) => shallowMount(FakeComponentOuter, opts);

describe('referencesProvider', () => {
it('provides a store with a default state', () => {
const outer = createOuter({});
const inner = outer.find(FakeComponentInner);
expect(inner.exists()).toBe(true);
expect(inner.props('references')).toEqual({});
});

it('provides references from a store', () => {
const outer = createOuter();
const inner = outer.find(FakeComponentInner);
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'],
},
});
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`
});
});
12 changes: 12 additions & 0 deletions tests/unit/stores/AppStore.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('AppStore', () => {
systemColorScheme: ColorScheme.light,
preferredLocale: null,
availableLocales: [],
includedArchiveIdentifiers: [],
});
});

Expand All @@ -37,6 +38,7 @@ describe('AppStore', () => {
systemColorScheme: ColorScheme.light,
preferredLocale: null,
availableLocales: [],
includedArchiveIdentifiers: [],
});

// restore target
Expand Down Expand Up @@ -72,11 +74,20 @@ describe('AppStore', () => {
});
});

describe('setIncludedArchiveIdentifiers', () => {
it('sets the included archive identifiers', () => {
const includedArchiveIdentifiers = ['foo', 'bar'];
AppStore.setIncludedArchiveIdentifiers(includedArchiveIdentifiers);
expect(AppStore.state.includedArchiveIdentifiers).toEqual(includedArchiveIdentifiers);
});
});

it('resets the state', () => {
AppStore.setImageLoadingStrategy(ImageLoadingStrategy.eager);
AppStore.setPreferredColorScheme(ColorScheme.auto);
AppStore.setSystemColorScheme(ColorScheme.dark);
AppStore.syncPreferredColorScheme();
AppStore.setIncludedArchiveIdentifiers(['a']);
AppStore.reset();

expect(AppStore.state).toEqual({
Expand All @@ -86,6 +97,7 @@ describe('AppStore', () => {
systemColorScheme: ColorScheme.light,
preferredLocale: null,
availableLocales: [],
includedArchiveIdentifiers: [],
});
});
});