feat(compiler): Implement error codes system onto compiler modules #726

jye-sf · 2018-10-10T22:18:42Z

Details

Creates the 'lwc-errors' module for error handling and normalization

Implements the error codes system onto lwc compiler modules. These include:

postcss-plugin-lwc
jest-transformer
babel-plugin-transform-lwc-class
lwc-template-compiler
lwc-compiler

Does this PR introduce a breaking change?

Yes
No

If yes, please describe the impact and migration path for existing applications:

jye-sf · 2018-10-10T22:19:50Z

packages/lwc-errors/.npmignore

@@ -0,0 +1,2 @@
+src/
+.*


lwc-errors module starts here

jye-sf · 2018-10-10T22:20:20Z

packages/lwc-errors/src/compiler/errors.ts

@@ -0,0 +1,90 @@
+import { LWCErrorInfo, templateString } from "../shared/utils";


Main error utilities live here.

jye-sf · 2018-10-10T22:21:10Z

packages/lwc-errors/src/errors-transform.js

@@ -0,0 +1,76 @@
+const errors = require('../dist/commonjs/index.js');


This babel transform was written for the engine modules, so it will be removed.

jye-sf · 2018-10-10T22:23:13Z

packages/lwc-errors/src/compiler/error-info/babel-plugin-lwc-class.ts

@@ -0,0 +1,215 @@
+import { Level } from "../../shared/utils";


Error code metadata starts here. Looking at the whole picture might help us decide how to organize the numeric codes.

jye-sf · 2018-10-10T22:24:43Z

packages/lwc-errors/src/compiler/error-info/babel-plugin-lwc-class.ts

+    INVALID_STATIC_OBSERVEDATTRIBUTES: {
+        code: 0,
+        message: "Invalid static property \"observedAttributes\". \"observedAttributes\" cannot be used to track attribute changes. Define setters for {0} instead.",
+        type: "path.buildCodeFrameError",


type was originally meant to help us surface typed errors, but doesn't seem to have any purpose if we're normalizing to CompilerError. If there are no concerns, I'll remove this property.

jye-sf · 2018-10-10T22:37:45Z

packages/lwc-errors/src/compiler/errors.ts

+    return new CompilerError(errorInfo.code, fullMessage, filename, location);
+}
+
+export function normalizeCompilerError(error: any, newContext?: CompilerContext): CompilerError {


normalizeToCompilerError?

jye-sf · 2018-10-10T22:42:49Z

packages/babel-plugin-transform-lwc-class/src/__tests__/api-decorator.spec.js

@@ -212,7 +212,7 @@ describe('Transform property', () => {
        }
    `, {
        error: {
-            message: 'test.js: Boolean public property must default to false.',
+            message: 'Boolean public property must default to false.',


Tests made to be more tolerant to changes to the overall error message.

caridy · 2018-10-11T02:28:47Z

this is a very very long PR, is there a doc somewhere to understand this better?

jye-sf · 2018-10-11T17:05:45Z

It's changed a little since then but we had a few discussions around the following docs and the linked resources:

Initial summary - https://salesforce.quip.com/3aSCA6NzEHcE
Followup discussion - https://salesforce.quip.com/VnlqAaiWdctb
More specific implementation questions - https://salesforce.quip.com/w4EjA5V5YNlY

diervo · 2018-10-11T20:21:38Z

this is a very very long PR, is there a doc somewhere to understand this better?

@caridy understands now how we feel with hist refactors :)

apapko

Here are few things that i would like to revisit:

Centralized vs Distributed Error Messages: should each package within the repo contain its own errors file instead of lwc-error owning all the errors? I feel like naming convention could be simplified if each package holds all its messages.
Naming convention scheme: If we do keep all the errors inside the lwc-errors, should each folder representing a sub-package match the name of the module it holds the errors for? ex: lwc-compiler errors will be represented as lwc-errors/src/lwc-compiler/errors.ts
Error type: should error type represent a package or an operation? If an invariant error is thrown from the compiler, is it sufficient to say that it was a 'transform' type? Or do we want to flag the origin, such as 'compiler'?
package structure: since everything is technically related to errors, do we need an extra error-info folder or will src//errors.ts be sufficient? ( current: lwc-errors/src/compiler/error-info/babel-plugin-lwc-class.ts)
should the generateCompilerError accept a message and a config object instead of explicitly listing all the params ( see comment below )?

apapko · 2018-10-11T18:27:39Z

packages/babel-plugin-transform-lwc-class/src/__tests__/api-decorator.spec.js

@@ -293,7 +293,7 @@ describe('Transform property', () => {
        }
    `, {
        error: {
-            message: 'test.js: Ambiguous attribute name tabindex. tabindex will never be called from template because its corresponding property is camel cased. Consider renaming to "tabIndex".',
+            message: 'Ambiguous attribute name tabindex. tabindex will never be called from template because its corresponding property is camel cased. Consider renaming to "tabIndex".',


pls wrap property name in quote? "tabindex"

same as above:

Ambiguous attribute name "tabindex". "tabindex" will never be called from template because its corresponding property is camel cased. Consider renaming to "tabIndex".'

apapko · 2018-10-11T18:28:03Z

packages/babel-plugin-transform-lwc-class/src/__tests__/api-decorator.spec.js

@@ -308,7 +308,7 @@ describe('Transform property', () => {
        }
    `, {
        error: {
-            message: 'test.js: Ambiguous attribute name maxlength. maxlength will never be called from template because its corresponding property is camel cased. Consider renaming to "maxLength".',
+            message: 'Ambiguous attribute name maxlength. maxlength will never be called from template because its corresponding property is camel cased. Consider renaming to "maxLength".',


packages/babel-plugin-transform-lwc-class/src/component.js

apapko · 2018-10-11T20:34:57Z

packages/lwc-compiler/src/compiler/options.ts

-        throw new TypeError(`Expected an object with files to be compiled.`);
-    }
+    invariant(
+        !isUndefined(options.files) && !!Object.keys(options.files).length,


i believe we can just do

Object.keys(options.files).length

as it is a truthy value;
not unless you explicitly expect a boolean.

Meaning !! not necessary

The invariant function does expect a boolean though. Is this worth changing the type to any?

packages/lwc-errors/src/errors-transform.js

packages/lwc-errors/src/shared/utils.ts

packages/lwc-errors/tsconfig.json

apapko · 2018-10-11T23:21:40Z

packages/lwc-template-compiler/src/index.ts

@@ -25,7 +25,7 @@ export default function compiler(

    let code = '';
    const warnings: CompilationWarning[] = [];
-
+// TODO ERROR CODES:


packages/postcss-plugin-lwc/src/selector-scoping/validate.ts

jye-sf · 2018-10-12T06:58:57Z

Some thoughts on your thoughts; we can discuss more tomorrow too:

Centralized vs Distributed Error Messages: should each package within the repo contain its own errors file instead of lwc-error owning all the errors? I feel like naming convention could be simplified if each package holds all its messages.

With a centralized package, it's easier to validate that each error code is unique when we compile the package and we can eventually have all the error-related documentation in one place.

The other benefit was being able to organize the errors independently from how the packages are organized. Looking at the whole thing though, the errors are mostly fairly specific to their packages, so we might not want to organize them independently anymore.

If we're organizing by package anyways, I don't see why each package can't just contain its own errors file. The main concern would be verifying the error codes.

Also, would the errors docs live with each package too?

Naming convention scheme: If we do keep all the errors inside the lwc-errors, should each folder representing a sub-package match the name of the module it holds the errors for? ex: lwc-compiler errors will be represented as lwc-errors/src/lwc-compiler/errors.ts

Yeah that's one way we can organize things if we want to go with a deeper file structure. I personally feel that this would be better than the flat directory (/src/everything-here) if we decide to organize by source package.

Error type: should error type represent a package or an operation? If an invariant error is thrown from the compiler, is it sufficient to say that it was a 'transform' type? Or do we want to flag the origin, such as 'compiler'?

The error instance itself should inherently provide the origin since it is a 'CompilerError'. For type, do we still need that property? Looking at the big picture, since we're normalizing to 'CompilerError', I'm not sure it's useful anymore. If we want to surface the error's origin sub-package, maybe that would be better served by a utility that lets each sub-package prepend some message to the error. Thoughts on that?

package structure: since everything is technically related to errors, do we need an extra error-info folder or will src//errors.ts be sufficient? ( current: lwc-errors/src/compiler/error-info/babel-plugin-lwc-class.ts)

I mainly did that so it's easier to distinguish the files purely holding the error metadata from the error-related code/utilities. If that's not helpful we can definitely flatten it again, but I think I prefer the deeper directory structure.

should the generateCompilerError accept a message and a config object instead of explicitly listing all the params ( see comment below )?

Yes, that's much better

caridy · 2018-10-17T16:43:01Z

who can do a walk thru this PR in a quick chat this week?

diervo · 2018-10-17T18:01:27Z

We should do it tomorrow

…

On Wed, Oct 17, 2018 at 9:43 AM Caridy Patiño ***@***.***> wrote: who can do a walk thru this PR in a quick chat this week? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#726 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH0qLMAxrmgxzAWxiiexzB8X29U_fLsks5ul14bgaJpZM4XWcFD> .

jye-sf · 2018-10-18T16:54:56Z

@caridy @diervo @apapko What time works for you today?

diervo · 2018-10-18T20:32:21Z

Whenever @caridy can, hopefully today? Otherwise I will do another final pass and merge it EOW

jye-sf · 2018-10-19T00:05:17Z

Sounds good. There are a few more changes that should be ready tonight, so no rush on that final pass.

jye-sf · 2018-10-19T07:26:59Z

Just realized a few hours ago that the remaining changes to expose the new error shape through lwc-compiler are going to break downstream modules expecting the old error shape (aura, platform-compiler). Going to add a temporary converter to maintain the existing API so that we don't need to do any coordinated releases on a huge PR like this.

Thoughts? Also, are aura and platform-compiler the only places that use lwc-compiler, or are there more?

diervo · 2018-10-19T16:32:22Z

@jye-sf don't quite follow that lets talk once you are in the office, or hangouts

jye-sf · 2018-10-19T17:29:57Z

The diagnostics are part of the lwc-compiler API, so we hit compilation errors in platform-compiler, and anything relying on the old 'location' info will break.

// Older shape
interface CompilerOutput {
    ...
    diagnostics: Diagnostic[];
}

interface Diagnostic {
    level: DiagnosticLevel;
    message: string;
    filename?: string;
    location?: Location;
}

interface Location {
    start: number;
    length: number;
}

// New shape
interface CompilerOutput {
    ...
    diagnostics: CompilerDiagnostic[];
}

interface CompilerDiagnostic {
    code: number;
    level: Level;
    message: string;
    filename?: string;
    location?: Location;
}

interface Location {
    line: number;
    column: number;
}

jye-sf · 2018-10-19T23:13:59Z

packages/lwc-template-compiler/typings/parse5.d.ts

+            startCol: number;
+        }
+    }
+}


Our parse5 version is older, while jsdom is using a newer version. This the right way to resolve it?

jye-sf · 2018-10-19T23:15:16Z

packages/postcss-plugin-lwc/typings/postcss.d.ts

+    export interface CssSyntaxError {
+        lwcCode?: number;
+    }
+}


Add property to library Error object so we can pass the error code.

jye-sf · 2018-10-19T23:15:21Z

packages/postcss-plugin-lwc/typings/error.d.ts

@@ -0,0 +1,3 @@
+declare interface Error {
+    lwcCode?: number
+}


Add property to library Error object so we can pass the error code.

apapko

Looks good! Jason and I have discussed all of the outstanding comments offline and concluded that we are going to hold off on some of them until the next pass. @jye-sf please create a git issue to track the future changes that we need to revisit.

salesforce-best-lwc-internal · 2018-10-23T04:14:42Z

Benchmark results

Base commit: ce341b8 | Target commit: 5a51cec

lwc-engine-benchmark

table-append-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table/append/1k	duration	146.90 (±3.90 ms)	147.30 (±4.00 ms)	+0.4ms (0.3%) 👌

table-clear-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table/clear/1k	duration	6.00 (±0.30 ms)	6.20 (±0.50 ms)	+0.2ms (3.3%) 👎

table-create-10k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table/create/10k	duration	862.15 (±6.35 ms)	864.60 (±5.60 ms)	+2.5ms (0.3%) 👎

table-create-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table/create/1k	duration	114.05 (±3.05 ms)	115.25 (±2.30 ms)	+1.2ms (1.1%) 👌

table-update-10th-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table/update-10th/1k	duration	77.50 (±4.50 ms)	75.55 (±2.35 ms)	-2.0ms (2.5%) 👍

tablecmp-append-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-component/append/1k	duration	236.75 (±10.45 ms)	236.05 (±14.10 ms)	-0.7ms (0.3%) 👌

tablecmp-clear-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-component/clear/1k	duration	11.70 (±1.30 ms)	11.75 (±1.25 ms)	+0.1ms (0.4%) 👌

tablecmp-create-10k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-component/create/10k	duration	1648.65 (±10.95 ms)	1637.85 (±10.85 ms)	-10.8ms (0.7%) 👍

tablecmp-create-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-component/create/1k	duration	196.90 (±4.45 ms)	199.80 (±6.05 ms)	+2.9ms (1.5%) 👌

tablecmp-update-10th-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-component/update-10th/1k	duration	71.40 (±5.20 ms)	71.70 (±4.50 ms)	+0.3ms (0.4%) 👌

wc-append-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-wc/append/1k	duration	231.85 (±8.80 ms)	237.20 (±12.60 ms)	+5.3ms (2.3%) 👌

wc-clear-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-wc/clear/1k	duration	23.40 (±2.05 ms)	20.40 (±2.00 ms)	-3.0ms (12.8%) 👍

wc-create-10k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-wc/create/10k	duration	1665.90 (±67.30 ms)	1734.70 (±38.80 ms)	+68.8ms (4.1%) 👎

wc-create-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-wc/create/1k	duration	200.15 (±6.60 ms)	204.90 (±5.40 ms)	+4.8ms (2.4%) 👎

wc-update-10th-1k	metric	base(`ce341b8`)	target(`5a51cec`)	trend
benchmark-table-wc/update-10th/1k	duration	72.65 (±4.85 ms)	70.15 (±4.05 ms)	-2.5ms (3.4%) 👌

jye-sf added enhancement gus-story nomerge labels Oct 10, 2018

jye-sf requested review from pmdartus and apapko October 10, 2018 22:18

jye-sf commented Oct 10, 2018

View reviewed changes

packages/lwc-errors/.npmignore

@@ -0,0 +1,2 @@

src/

.*

Copy link

Contributor Author

jye-sf Oct 10, 2018

Choose a reason for hiding this comment

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

lwc-errors module starts here

jye-sf commented Oct 10, 2018

View reviewed changes

apapko requested changes Oct 11, 2018

View reviewed changes

jye-sf force-pushed the jye/error-codes-compiler branch from 016fd49 to f7eabb4 Compare October 18, 2018 09:28

apapko changed the title ~~wip feat(compiler): Implement error codes system onto compiler modules~~ feat(compiler): Implement error codes system onto compiler modules Oct 18, 2018

jye-sf commented Oct 19, 2018

View reviewed changes

jye-sf added 17 commits October 22, 2018 00:04

fix: type organization in lwc-errors

fd46b83

fix: alphabetize errors

287a901

feat: add location and filename to lwc-template-compiler

7b312cf

feat: move error wrapping for postcss to compiler boundary

3f10ab5

fix: commit typings files

83785a8

feat: move error wrapping for lwc-jest-transformer

f1904e4

feat: move error wrapping for babel plugin

0dcc000

feat: implement error codes for template-compiler warnings and cleanup

b79cd1c

feat: refactor warnOnElement in template-compiler

f88486d

feat: update test fixtures and cleanup

0552603

fix: fixup

e75fd53

feat(compiler): normalize at compiler level and update tests

5937ec1

fix: remove debuggers

a40249a

fix: add temporary adapter to prevent breakage

6dbd09d

fix: minor cleanups

87e2b23

feat: renames and refactors from code review

c12c9e3

feat: update version

828e9b8

jye-sf force-pushed the jye/error-codes-compiler branch from 0148960 to 828e9b8 Compare October 22, 2018 16:21

jye-sf and others added 5 commits October 22, 2018 10:31

fix: update tests

ac6335e

fix: update versions

db36eef

fix: merge issues

86fc11f

Merge branch 'master' into jye/error-codes-compiler

de471ab

fix: move constant and add optional properties for temp compatibility

2b56b0e

jye-sf removed the nomerge label Oct 23, 2018

apapko approved these changes Oct 23, 2018

View reviewed changes

jye-sf added 2 commits October 22, 2018 18:16

fix: update fake codes to be more representative of the real ones

c5d727b

fix: test coverage

5a51cec

jye-sf merged commit 3ee81d5 into master Oct 23, 2018

diervo deleted the jye/error-codes-compiler branch October 24, 2018 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): Implement error codes system onto compiler modules #726

feat(compiler): Implement error codes system onto compiler modules #726

jye-sf commented Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

jye-sf Oct 10, 2018

caridy commented Oct 11, 2018

jye-sf commented Oct 11, 2018

diervo commented Oct 11, 2018 •

edited

Loading

apapko left a comment •

edited

Loading

apapko Oct 11, 2018

apapko Oct 21, 2018

apapko Oct 11, 2018

apapko Oct 11, 2018

apapko Oct 21, 2018

jye-sf Oct 22, 2018

apapko Oct 11, 2018

jye-sf commented Oct 12, 2018

caridy commented Oct 17, 2018

diervo commented Oct 17, 2018 via email

jye-sf commented Oct 18, 2018

diervo commented Oct 18, 2018

jye-sf commented Oct 19, 2018

jye-sf commented Oct 19, 2018

diervo commented Oct 19, 2018

jye-sf commented Oct 19, 2018

jye-sf Oct 19, 2018

jye-sf Oct 19, 2018

jye-sf Oct 19, 2018

apapko left a comment

salesforce-best-lwc-internal bot commented Oct 23, 2018

		@@ -0,0 +1,90 @@
		import { LWCErrorInfo, templateString } from "../shared/utils";

		@@ -0,0 +1,76 @@
		const errors = require('../dist/commonjs/index.js');

		@@ -0,0 +1,215 @@
		import { Level } from "../../shared/utils";

feat(compiler): Implement error codes system onto compiler modules #726

feat(compiler): Implement error codes system onto compiler modules #726

Conversation

jye-sf commented Oct 10, 2018

Details

Does this PR introduce a breaking change?

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

caridy commented Oct 11, 2018

jye-sf commented Oct 11, 2018

diervo commented Oct 11, 2018 • edited Loading

apapko left a comment • edited Loading

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

jye-sf commented Oct 12, 2018

caridy commented Oct 17, 2018

diervo commented Oct 17, 2018 via email

jye-sf commented Oct 18, 2018

diervo commented Oct 18, 2018

jye-sf commented Oct 19, 2018

jye-sf commented Oct 19, 2018

diervo commented Oct 19, 2018

jye-sf commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapko left a comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Oct 23, 2018

Benchmark results

lwc-engine-benchmark

diervo commented Oct 11, 2018 •

edited

Loading

apapko left a comment •

edited

Loading