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

Projects
None yet
2 participants
@eug48
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Jul 31, 2017

Owner

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.

Owner

jamesagnew commented Jul 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@eug48

eug48 Aug 1, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@eug48

eug48 Aug 1, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Aug 1, 2017

Owner

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.

Owner

jamesagnew commented Aug 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@eug48

eug48 Aug 17, 2017

Contributor

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

Contributor

eug48 commented Aug 17, 2017

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

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Aug 25, 2017

Owner

Awesome! :)

Owner

jamesagnew commented Aug 25, 2017

Awesome! :)

@jamesagnew jamesagnew merged commit 5efeb12 into jamesagnew:master Aug 25, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

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