Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Convert external links to other enabled docs to internal links #714

Closed
wants to merge 2 commits into from
Closed

Convert external links to other enabled docs to internal links #714

wants to merge 2 commits into from

Conversation

j-f1
Copy link
Contributor

@j-f1 j-f1 commented Nov 4, 2017

Fixes #234 and might fix the issues with the internal PHP doc links.

How it works:

  • Each entry in index.json gets a url key linking to the page where the doc was originally scraped from.
  • An app.Urls instance in the frontend is passed each Doc.
  • It pulls out each entry and maps from its external URL to its internal URL.
  • When rendering a documentation page, external URLs are run through app.urls.get, which returns the internal URL for the external URL, if one is available

@j-f1 j-f1 force-pushed the internalize-links branch from 0f6cef2 to 1385eba Compare November 4, 2017 11:48
@j-f1 j-f1 force-pushed the internalize-links branch from 1385eba to 344dbf6 Compare November 4, 2017 18:11
j-f1
j-f1 commented Nov 5, 2017
View reviewed changes
lib/docs/filters/core/entries.rb
Entry.new(name, path, type)
if frag.present? && frag.include?('#')
path = frag
# TODO: What should `url` get changed to?

This comment was marked as off-topic.

Sign in to view

@Thibaut
Copy link
Member

Thibaut commented Mar 25, 2018

This is a great PR, but I'm worried about the performance penalty + the fact that this significantly increases the size of the index.json files. I'm not sure the benefit is worth that penalty :/

@j-f1
Copy link
Contributor Author

j-f1 commented Mar 26, 2018

Using a compressed trie would probably decrease download size, but I’m not sure what effect it would have on client-side performance.

@jmerle jmerle added the feature label May 2, 2019
@jmerle jmerle mentioned this pull request May 23, 2019
Merged
@jmerle
Copy link
Contributor

jmerle commented Jun 2, 2019 •
edited
Loading

The code looks good, but I am with Thibaut on this one. After some testing, it seems like the index.json does increase quite a bit:

Underscore.js before: 8,5K
Underscore.js after: 15K

OpenJDK 8 before: 1,8M
OpenJDK 8 after: 3,0M

OpenJDK 8 GUI before: 1,9M
OpenJDK 8 GUI after: 3,1M

OpenJDK 8 Web before: 906K
OpenJDK 8 Web after: 1,5M

I also ran some benchmarks. I'm not sure if my benchmarking method is somewhat good, but here's how I tested it:

  1. Refresh DevDocs with OpenJDK 8, OpenJDK 8 GUI and OpenJDK 8 Web disabled 5 times, noting down the times in the console.
  2. Enable OpenJDK 8, OpenJDK 8 GUI and OpenJDK 8 Web (the before versions) and refresh 5 times, noting down the times in the console.
  3. Enable OpenJDK 8, OpenJDK 8 GUI and OpenJDK 8 Web (the after versions) and refresh 5 times, noting down the times in the console.

This gave the following results:

Init/Load/Start/Total

Disabled:
1. 20ms/132ms/10ms/162ms
2. 21ms/123ms/9ms/153ms
3. 19ms/118ms/8ms/145ms
4. 21ms/121ms/10ms/152ms
5. 18ms/118ms/9ms/145ms
Average: 19.8ms/122.4ms/9.2ms/151.4ms

Enabled before:
1. 20ms/295ms/15ms/330ms
2. 17ms/287ms/13ms/317ms
3. 20ms/292ms/13ms/325ms
4. 18ms/291ms/14ms/323ms
5. 19ms/304ms/14ms/337ms
Average: 18.8ms/293.8ms/13.8ms/326.4ms

Enabled after:
1. 18ms/349ms/609ms/976ms
2. 19ms/337ms/567ms/923ms
3. 18ms/334ms/540ms/892ms
4. 18ms/333ms/549ms/900ms
5. 18ms/355ms/564ms/937ms
Average: 18.2ms/341.6ms/565.8ms/925.6ms

Performance seems quite badly impacted, and I think it'll only be worse when the user has more documentations enabled.

Furthermore, I don't know if it is working correctly on FileParser's where the base url defaults to localhost (like the OpenJDK scraper does):

{
    "name":"ZoneRulesProvider.provideZoneIds()",
    "path":"java/time/zone/zonerulesprovider#provideZoneIds--",
    "type":"java.time.zone",
    "url":"http://localhost/java/time/zone/ZoneRulesProvider.html#provideZoneIds--"
}

Not exactly sure whether localhost links should be added or not.

Overall it's a cool feature, but I don't think the performance impact and the increase in index.json size is worth it (where the performance is the biggest problem). I don't think we should merge this until the performance issues are solved.

@jmerle
Copy link
Contributor

jmerle commented Aug 11, 2019

Closing this for now.

@jmerle jmerle closed this Aug 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Intercept links to known documentation
3 participants
@j-f1 @Thibaut @jmerle