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

crm-ui-debug - If in debug mode, then load pretty-printer for JSON data #18994

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

totten
Copy link
Member

@totten totten commented Nov 19, 2020

Overview

The Angular directive crm-ui-debug is used to conditionally dump debug details on an AngularJS-based page (i.e. if the user requests ?angularDebug=1). This patch improves the formatting.

Before

The debug data is serialized to JSON and dumped as a large text blob (<pre>{{myData|json}}</pre>).

Screen Shot 2020-11-19 at 1 38 31 AM

For objects with deep graphs, it's hard to see the structure.

After

The debug data is printed as a pretty tree with highlighting and expandable/collapsible sections.

Screen Shot 2020-11-19 at 1 38 04 AM

Technical Details

The pretty printer adds ~10kb to every page that uses crmUi (ie every Angular page in Civi), which seems a little gratuitious for production page-views. Alas, the angularDebug=1 preference is a client-side thing that we don't know about server-side, so there's no way to anticipate if it's worth sending.

As a mitigation, this patch consults the global debug setting (Civi::setting()->get('debug_enabled')):

  • If the server has debug enabled, then it sends the pretty-print library.
  • If the server does not have debug enabled, then it sends the el-cheapo version (<pre>{{myData|json}}</pre>).

CC @colemanw @seamuslee001

@civibot
Copy link

civibot bot commented Nov 19, 2020

(Standard links)

@civibot civibot bot added the master label Nov 19, 2020
@colemanw
Copy link
Member

Looks good @totten but I think test failures relate

@totten
Copy link
Member Author

totten commented Nov 19, 2020

@colemanw Indeed it is. Rebased.

@colemanw
Copy link
Member

@totten one more failure...

$isDebug ? 'bower_components/json-formatter/dist/json-formatter.min.js' : 'ang/jsonFormatter.stub.js',
],
'css' => [
$isDebug ? 'bower_components/json-formatter/dist/json-formatter.min.css' : NULL,
Copy link
Member

Choose a reason for hiding this comment

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

The coupling between $isDebug and the ability of this thing to function feels hack-ish to me. For the purposes at hand, sure it makes sense, but what if some other module wanted this functionality for purposes other than debugging? I might even have a use for it in search kit.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to trigger more work with that comment. This whole PR feels like a low-priority side-project and we should get back to the main tasks. If you fix the test I'll merge it as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can see how jsonFormatter could also be useful in admin/site-builder tooling.

Found a way to remove the jsonFormatter stub. With the update, jsonFormatter works like a regular module. (So if you want to use, then just... require it...) The optimization bit is now in crmUiDebug, which has a soft-requirement jsonFormatter.

Tangentially, jsonFormatter will also autoload now if used by an afform.

@totten totten force-pushed the master-ang-json branch 2 times, most recently from 834ae97 to 2035ad5 Compare November 19, 2020 20:24
@eileenmcnaughton eileenmcnaughton merged commit 32cdb5a into civicrm:master Nov 19, 2020
@totten totten deleted the master-ang-json branch November 20, 2020 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants