FED-466: Fix console logs that require formatting #52

kealjones-wk · 2022-10-14T21:32:10Z

Motivation

These logs suck and are no help:

Changes

Add a dart "formatter" (based on an old version of node js's console format.... the [newer one is scary])(https://github.com/nodejs/node/blob/main/lib/internal/util/inspect.js#L2173-L2287)
- Result:

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary2-wf · 2022-10-14T21:33:35Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

greglittlefield-wf

Some comments, but this looks awesome!!

lib/src/util/console_log_utils.dart

test/unit/console_log_utils_test.dart

lib/src/util/console_log_utils.dart

greglittlefield-wf · 2022-10-14T22:36:11Z

lib/src/util/console_log_utils.dart

@@ -152,3 +154,51 @@ class ConsoleConfig {
  /// Captures calls to `console.log`, `console.warn`, and `console.error`.
  static const ConsoleConfig all = ConsoleConfig._('all');
 }
+
+final formatRegExp = RegExp('%[sdj%]');
+String format(dynamic f, List<dynamic> arguments) {


This could use a doc comment

greglittlefield-wf · 2022-10-19T16:54:21Z

lib/src/util/console_log_formatter.dart

+
+final _formatRegExp = RegExp('%[sdj%]');
+
+/// A doc comment


You want me to close this PR? 😛

🖕 get out of here friday greg its wednesday.

This is worse than no doc comment lol

greglittlefield-wf · 2022-10-19T16:57:39Z

test/unit/console_log_utils_test.dart

+          // the test package.
+          final printCalls = recordPrintCalls(() {
+            printConsoleLogs(() {
+              callMethod(getProperty(window, 'console'), 'log', ['%s World Number %d! %j', 'Hello']);


Looks like this is missing the jsified argument, the number, and the additional arg

greglittlefield-wf · 2022-10-19T16:57:44Z

lib/src/util/console_log_formatter.dart

+          return num.tryParse(arguments[i++].toString()).toString();
+        case '%j':
+          try {
+            return _jsonStringify([arguments[i++]]);


Since this isn't callMethod, it shouldn't be wrapped in a list.

Suggested change

return _jsonStringify([arguments[i++]]);

return _jsonStringify(arguments[i++]);

greglittlefield-wf · 2022-10-19T17:00:18Z

test/unit/console_log_utils_test.dart

@@ -223,8 +267,7 @@ void main() {
    if (runtimeSupportsPropTypeWarnings()) {
      group('captures errors correctly', () {
        test('when mounting', () {
-          final logs = recordConsoleLogs(() => rtl.render(Sample({'shouldAlwaysBeFalse': true}) as ReactElement),
-              configuration: ConsoleConfig.error);
+          final logs = recordConsoleLogs(() => rtl.render(Sample({'shouldAlwaysBeFalse': true}) as ReactElement), configuration: ConsoleConfig.error);


Looks like these got formatted differently by mistake

they bug me... so i re-formatted them in the next commit.

greglittlefield-wf

Just a couple more things

greglittlefield-wf · 2022-10-19T21:11:55Z

lib/src/util/console_log_formatter.dart

@@ -53,7 +55,9 @@ String format(dynamic f, List<dynamic> arguments) {
          return num.tryParse(arguments[i++].toString()).toString();
        case '%j':
          try {
-            return _jsonStringify([arguments[i++]]);
+            final argToStringify = arguments[i++];
+            debugger();


Looks like this and the import were committed by mistake.

greglittlefield-wf · 2022-10-19T21:14:53Z

lib/src/util/console_log_utils.dart

-      consoleRefs[config].apply([message, arg1, arg2, arg3, arg4, arg5], thisArg: self);
-    });
+    consolePropertyDescriptors[config] = _getOwnPropertyDescriptor(_console, config);
+    consoleRefs[config] = context['console'][config];


Could we restore the cast that was previously here along with the typing on consoleRefs?

- itz dumb and i dont like it.

greglittlefield-wf

+10

lib/src/util/console_log_utils.dart

greglittlefield-wf

+10 assuming CI is all green

greglittlefield-wf · 2022-10-20T23:52:34Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

fix console logs that require formatting

aa96e2f

rmconsole-wf added the Merge Requirements Unmet label Oct 14, 2022

add 1 test for formatter :P

c6fde52

kealjones-wk requested review from a team and aaronlademann-wf and removed request for a team October 14, 2022 21:48

kealjones-wk added 2 commits October 14, 2022 14:53

fix json stringify

8f7b9d4

add more situations to the formatter test

19b2de9

greglittlefield-wf requested changes Oct 14, 2022

View reviewed changes

address gregs feedback

5eeb3a0

kealjones-wk requested a review from greglittlefield-wf October 14, 2022 23:07

add exact number of args test

6790c0d

greglittlefield-wf requested changes Oct 19, 2022

View reviewed changes

kealjones-wk added 2 commits October 19, 2022 13:31

address feedback

800eeb3

add doc comment, and add all options from spec

2e3d986

greglittlefield-wf requested changes Oct 19, 2022

View reviewed changes

kealjones-wk added 4 commits October 19, 2022 14:33

change console getter to final in function

cfc5f88

fix typing

2b39903

fix javascript function call for dart2js

c7beb86

- itz dumb and i dont like it.

add // @Dart = 2.7 to head of formatter file

80acd97

kealjones-wk requested a review from greglittlefield-wf October 20, 2022 22:25

greglittlefield-wf previously approved these changes Oct 20, 2022

View reviewed changes

lib/src/util/console_log_utils.dart Outdated Show resolved Hide resolved

kealjones-wk added 2 commits October 20, 2022 16:18

Merge branch 'master' into spike-fed-466

3990a38

use the this argument for jsfunctionapply

b814c35

kealjones-wk dismissed greglittlefield-wf’s stale review via b814c35 October 20, 2022 23:28

greglittlefield-wf approved these changes Oct 20, 2022

View reviewed changes

greglittlefield-wf added the semver: Patch label Oct 20, 2022

rmconsole-wf added the RM Ready label Oct 20, 2022

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Oct 20, 2022

rmconsole-wf approved these changes Oct 20, 2022

View reviewed changes

rmconsole7-wk merged commit 434df25 into master Oct 20, 2022

rmconsole7-wk deleted the spike-fed-466 branch October 20, 2022 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-466: Fix console logs that require formatting #52

FED-466: Fix console logs that require formatting #52

kealjones-wk commented Oct 14, 2022 •

edited

Loading

aviary2-wf commented Oct 14, 2022

greglittlefield-wf left a comment

greglittlefield-wf Oct 14, 2022

greglittlefield-wf Oct 19, 2022

kealjones-wk Oct 19, 2022

greglittlefield-wf Oct 19, 2022

greglittlefield-wf Oct 19, 2022

greglittlefield-wf Oct 19, 2022

greglittlefield-wf Oct 19, 2022

kealjones-wk Oct 19, 2022

greglittlefield-wf left a comment

greglittlefield-wf Oct 19, 2022

greglittlefield-wf Oct 19, 2022

greglittlefield-wf left a comment

greglittlefield-wf left a comment

greglittlefield-wf commented Oct 20, 2022

rmconsole-wf left a comment

	return _jsonStringify([arguments[i++]]);
	return _jsonStringify(arguments[i++]);

FED-466: Fix console logs that require formatting #52

FED-466: Fix console logs that require formatting #52

Conversation

kealjones-wk commented Oct 14, 2022 • edited Loading

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary2-wf commented Oct 14, 2022

Security Insights

Action Items

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Oct 20, 2022

rmconsole-wf left a comment

Choose a reason for hiding this comment

kealjones-wk commented Oct 14, 2022 •

edited

Loading