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

Conversation

mportiz08
Copy link
Contributor

@mportiz08 mportiz08 commented Jul 2, 2024

Bug/issue #, if applicable: 118834404

Summary

This adds logic to inactivate links to pages from other targets unless they have been included in the same combined archive.

Dependencies

swiftlang/swift-docc#821

Testing

You can use the archives from this fixture for the convenience of testing purposes:
archives.zip

Steps:

  1. Run VUE_APP_DEV_SERVER_PROXY=/path/to/Combined\ ExternalLinks\ Documentation.doccarchive npm run serve and open http://localhost:8080/documentation/outer/outerclass/dosomething(with:)
  2. Verify that all the links to "Inner" pages work, including in the declaration
  3. Run VUE_APP_DEV_SERVER_PROXY=/path/to/Outer.doccarchive npm run serve and open http://localhost:8080/documentation/outer/outerclass/dosomething(with:)
  4. Verify that all the links to "Inner" pages are no longer linked
  5. Check for any other regressions with links and linked declaration types using other fixture content.

Known issues:
Due to the way that the Render JSON and Navigator JSON are loaded independently by the renderer, it's possible that the links in step 4 may be initially linked for a very brief moment until the navigator has finished loading. I've discussed this with @d-ronnqvist, and this limitation is acceptable for now given the way that things are structured.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

mportiz08 added 2 commits July 8, 2024 14:41
Instead of passing a new boolean flag to indicate which refs should be
included or not, just strip out the URL data for refs that are not part
of included archives and should not be linked to.
@mportiz08 mportiz08 requested a review from hqhhuang July 9, 2024 20:53
@mportiz08 mportiz08 changed the title Inactivate known external links [alternate approach] Inactivate known external links Jul 9, 2024
Copy link
Contributor

@hqhhuang hqhhuang left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this approach!

I did a quick pass in docc-render and found that Mention.vue also uses the Reference component and has a required url prop. Should we also make that prop optional? Although it doesn't really make sense to render a title without a link in this section. @d-ronnqvist What's the plan for the "mentioned in" section for multi-target docs?

}

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

@d-ronnqvist
Copy link
Contributor

FIY: this doesn't depend on swiftlang/swift-docc-plugin#84. If anything it depends on swiftlang/swift-docc#821 which was merged in February.

@mportiz08
Copy link
Contributor Author

FIY: this doesn't depend on swiftlang/swift-docc-plugin#84. If anything it depends on swiftlang/swift-docc#821 which was merged in February.

Thanks, updated

@hqhhuang
Copy link
Contributor

@swift-ci test

@hqhhuang hqhhuang merged commit 9c05f4b into swiftlang:main Jul 15, 2024
1 check passed
hqhhuang added a commit to hqhhuang/swift-docc-render that referenced this pull request Jul 15, 2024
Inactivate known external links (swiftlang#878) rdar://118834404

---------

Co-authored-by: Hanqing Huang <[email protected]>
@mportiz08 mportiz08 mentioned this pull request Sep 18, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants