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.

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

ResponseHighlighter - workaround for next/previous links in multi-page responses #700

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

eug48
Copy link
Contributor

@eug48 eug48 commented Jul 31, 2017

A nice use-case for the new javascript links is being able to navigate multi-page responses just by clicking on the "next" links. However that currently doesn't work because the _getpages handler adds a "_format=xml" to its next/previous links so when they're clicked the response is downloaded rather than formatted through the ResponseHighlighterInterceptor.

This workaround just removes the "_format=xml" on client-side. I wasn't game enough to modify _getpages in case that has some backwards compatibility implication..

@jamesagnew
Copy link
Collaborator

I @eug48 , this issue is actually already fixed on the hapi3_refactor branch, which will be landed hopefully this week. There was a bug in the code that generated the link where if it saw application/xml in the browser's Accept header it assumed you wanted XML.

On a related note, I'm thinking about swapping the JavaScript logic for link generation to be handled on the server side.. I'm noticing that for very large responses, having the JavaScript processing happen causes a long delay before anything gets rendered (e.g. this search shows the line numbers but no contents for 5-10 seconds on my MacBook.

@eug48
Copy link
Contributor Author

eug48 commented Aug 1, 2017

Ok, that's great.

Yes, performance is indeed not good for huge responses but from what I can tell with the Chrome DevTools the vast majority of the time is spent rendering the large number of DOM nodes. I've just pushed a commit to this PR that logs performance and I get numbers like:

updateHyperlinksAndStyles: 1063.676025390625ms
User-perceived page loading time: 37160ms

Disabling JavaScript didn't improve things much. I also tried removing all the CSS but that wasn't it either. However, Firefox is 2-3 times faster than Chrome.

@eug48
Copy link
Contributor Author

eug48 commented Aug 1, 2017

A possible solution may be to do the syntax highlighting on the client by integrating code from a project like https://github.com/callumlocke/json-formatter or https://ace.c9.io/ - they're able to highlight even large responses fairly quickly.

@jamesagnew
Copy link
Collaborator

Oh man, you're absolutely right.. I was contemplating breaking the cardinal rule of optimization: measure first. :)

It seems like most of the time is simply spent doing the initial paint, so nothing server side would help there.

I'm going to leave this open until the hapi3_refactor branch lands, then merge after that time. We probably don't need the XML format fix at that point (since that part is already fixed server side) but the logging is good anyhow.

@eug48
Copy link
Contributor Author

eug48 commented Aug 17, 2017

Cool, I've rebased my branch to only include the perf logging. The next & prev links now work :)

@jamesagnew
Copy link
Collaborator

Awesome! :)

@jamesagnew jamesagnew merged commit 5efeb12 into hapifhir:master Aug 25, 2017
jamesagnew added a commit that referenced this pull request Aug 25, 2017
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.

2 participants