fix(engine): identify LWC console logs #674
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caridy
approved these changes
Sep 25, 2018
There was a problem hiding this comment.
I'm fine with this but:
- there are test failures
- LWC is a brand that is not ready to be public, adding this to the logs mean that people will start asking about LWC even though they are only using Aura on their end. /cc @diervo
pmdartus
requested changes
Sep 25, 2018
| @@ -67,7 +67,7 @@ const assert = { | |||
| throw new Error(msg); | |||
| }, | |||
| logError(message: string, elm: Element | null) { | |||
There was a problem hiding this comment.
Change type of the logError and logWarning function to be undefined instead of null:
function logWarning(message: string, elm?: Element) {}There was a problem hiding this comment.
I did a quick code search in the engine to make sure we are not missing any error message, there is still console.error in def.ts. Can you make the change and add it to this PR?
| @@ -67,7 +67,7 @@ const assert = { | |||
| throw new Error(msg); | |||
| }, | |||
| logError(message: string, elm: Element | null) { | |||
| let msg = message; | |||
| let msg = `[LWC err]: ${message}`; | |||
There was a problem hiding this comment.
What do you think about using error and warning there, instead of the shorthand?
jodarove
force-pushed
the
jodarove/add-header-to-console-logs
branch
from
72f857f
to
4a54e71
Compare
pmdartus
approved these changes
Sep 26, 2018
jodarove
force-pushed
the
jodarove/add-header-to-console-logs
branch
2 times, most recently
from
c6f58c5
to
b1a34f9
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
@jodarove LGTM |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Adds [LWC error/warning] prefix to clearly identify LWC's console logs
jodarove
force-pushed
the
jodarove/add-header-to-console-logs
branch
from
a3c9a5b
to
2242add
Compare
Benchmark resultsBase commit: |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Adds [LWC err/warn] prefix in
assert.logErrorandassert.logWarningto clearly identify LWC's console logsDoes this PR introduce a breaking change?