Prevent BindEvaluator from returning Error objects to web worker #7599

kmh287 · 2017-02-16T14:22:10Z

/to @choumx @jridgewell

I'm not excited about the loss of information here. Any suggestions on how to provide more detailed information?

Fixes #7598

dreamofabear

bind-impl.js also needs to be changed
As discussed, pass back the stack trace and any other useful information in a new clone-friendly typedef

jridgewell · 2017-02-16T22:06:16Z

As discussed, pass back the stack trace and any other useful information in a new clone-friendly typedef

+100.

…ixes

kmh287 · 2017-02-17T18:52:41Z

Looks like stack contains both the message and a stack trace. I think the best approach here would be to simply return that, or just message if stack is unavailable as it isn't standard. If we go with that, then the typedef seems unnecessary to wrap a single string.

Also, bind-impl takes the value in the error mappings returned by the evaluator's functions and calls #createError on them, which can take either Error or string objects.

jridgewell · 2017-02-17T18:58:49Z

Looks like stack contains both the message and a stack trace

We mutate the error message to mark them as either "dev" or "user". Stack is unchanged by that.

kmh287 · 2017-02-17T19:01:03Z

I'm sorry I don't follow.

jridgewell · 2017-02-17T19:02:29Z

We need both if we want to reconstruct an Error and treat it normally (marking it user error so we don't receive it in the logs) on AMP doc side.

kmh287 · 2017-02-17T19:03:54Z

So you're saying create a user error with just the message and a dev error with the stack?

jridgewell · 2017-02-17T19:08:44Z

We can reconstruct on our side:

const { message, stack } = receivedMessage;
const error = new Error(message);
error.stack = stack;

// Treat it however
throw error;
user().error(error);

kmh287 · 2017-02-17T20:02:04Z

I hope I've addressed your concerns, PTAL @choumx @jridgewell

…ixes

dreamofabear · 2017-02-21T17:39:06Z

src/web-worker/web-worker.js

@@ -60,5 +60,7 @@ self.addEventListener('message', function(event) {

  /** @type {FromWorkerMessageDef} */
  const message = {method, returnValue, id};
+  // `message` may only contain vlaues or objects handled by the


s/vlaues/values

dreamofabear · 2017-02-21T17:40:22Z

src/web-worker/web-worker.js

@@ -60,5 +60,7 @@ self.addEventListener('message', function(event) {

  /** @type {FromWorkerMessageDef} */
  const message = {method, returnValue, id};
+  // `message` may only contain vlaues or objects handled by the
+  // structured clone algorithm.


Add link to https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm here.

dreamofabear · 2017-02-21T17:49:25Z

src/web-worker/amp-worker.js

+ *   stack: string,
+ * }}
+ */
+export let AmpWorkerErrorDef;


IMO this typedef shouldn't live here since:

It's not actually used by any code in this file and

It adds an unnatural dependency to bind-evaluator (the worker's use of bind-evaluator is an implementation detail, not vice-versa)

Move this to bind-evaluator since this only concerns the boundary between that class and bind-impl. Also, I think the helper methods below are simple enough to be unnecessary and not worthwhile as a layer of indirection.

How does this create a dependency on bind-evaluator?

Also, what happens if/when more functionality depends on the worker. This prevents everyone who uses the worker from rolling their own error type.

How does this create a dependency on bind-evaluator?

I meant bind-evaluator shouldn't depend on amp-worker.

Also, what happens if/when more functionality depends on the worker. This prevents everyone who uses the worker from rolling their own error type.

I'm not so sure that future use cases will need to return Error objects. bind-impl's usage of reportError is pretty particular.

dreamofabear · 2017-02-22T23:41:36Z

extensions/amp-bind/0.1/bind-impl.js

@@ -197,7 +197,10 @@ export class Bind {
      Object.keys(parseErrors).forEach(expressionString => {
        const elements = this.expressionToElements_[expressionString];
        if (elements.length > 0) {
-          const err = user().createError(parseErrors[expressionString]);
+          const parseError = parseErrors[expressionString];
+          const fullError = new Error(parseError.message);


Nit: s/fullError/error

dreamofabear · 2017-02-22T23:41:46Z

extensions/amp-bind/0.1/bind-impl.js

+          const parseError = parseErrors[expressionString];
+          const fullError = new Error(parseError.message);
+          fullError.stack = parseError.stack;
+          const err = user().createError(fullError);


Nit: s/err/userError

dreamofabear · 2017-02-22T23:42:39Z

extensions/amp-bind/0.1/bind-evaluator.js

+ *   stack: string,
+ * }}
+ */
+export let EvaluatorErrorDef;


Does this need to be exported?

jridgewell · 2017-02-23T20:11:18Z

extensions/amp-bind/0.1/bind-impl.js

+          const parseError = parseErrors[expressionString];
+          const error = new Error(parseError.message);
+          error.stack = parseError.stack;
+          const userError = user().createError(error);


Nit: You can pass parseError.message directly into #createError, and set #stack on the returned error.

dreamofabear · 2017-02-28T00:34:28Z

extensions/amp-bind/0.1/bind-evaluator.js

-        errors[expressionString] = new Error(
-            `Expression "${expressionString} is not cached."`);
+        const error =
+          new Error(`Expression "${expressionString}"" is not cached.`);


dreamofabear · 2017-02-28T00:34:34Z

extensions/amp-bind/0.1/bind-evaluator.js

-        errors[expressionString] = new Error(
-            `"${result}" is not a valid result for [${property}].`);
+        const error =
+          new Error(`"${result}" is not a valid result for [${property}].`);


dreamofabear · 2017-02-28T00:34:55Z

extensions/amp-bind/0.1/bind-impl.js

        const elements = this.expressionToElements_[expressionString];
        if (elements.length > 0) {
+          const evalError = errors[expressionString];
+          const err = user().createError(evalError.message);


Nit: userError to be consistent with above.

dreamofabear · 2017-02-28T00:44:54Z

@kmh287 You can address those nits in a follow-up. Merging now.

* Prevent BindEvaluator from returning error objects to web worker * cleanup * pr comments * cleanup * cleanup * lint error * added tests and addressed error from CI * pr comments * pr comments * pr comments

…project#7599) * Prevent BindEvaluator from returning error objects to web worker * cleanup * pr comments * cleanup * cleanup * lint error * added tests and addressed error from CI * pr comments * pr comments * pr comments

kmh287 added 3 commits February 16, 2017 09:16

Prevent BindEvaluator from returning error objects to web worker

0f2e1ee

cleanup

8c04450

Merge branch 'master' into bind_worker_fixes

f0154c5

kmh287 requested a review from dreamofabear February 16, 2017 16:12

dreamofabear requested changes Feb 16, 2017

View reviewed changes

Merge branch 'master' of github.com:kmh287/amphtml into bind_worker_f…

701c337

…ixes

pr comments

edd9e8b

kmh287 force-pushed the bind_worker_fixes branch from 219d820 to edd9e8b Compare February 17, 2017 19:52

kmh287 added 2 commits February 17, 2017 14:59

cleanup

acd470f

cleanup

b1469ec

kmh287 added 3 commits February 17, 2017 15:08

lint error

345b5ad

Merge branch 'master' of github.com:kmh287/amphtml into bind_worker_f…

bd22f07

…ixes

added tests and addressed error from CI

d994ef4

kmh287 mentioned this pull request Feb 21, 2017

Weekly Status 2017-02-20 #7679

Closed

dreamofabear requested changes Feb 21, 2017

View reviewed changes

pr comments

2827588

kmh287 force-pushed the bind_worker_fixes branch from 479861a to 2827588 Compare February 22, 2017 16:05

dreamofabear approved these changes Feb 22, 2017

View reviewed changes

pr comments

5c205fc

jridgewell reviewed Feb 23, 2017

View reviewed changes

pr comments

6e5af94

kmh287 force-pushed the bind_worker_fixes branch from 6b6c2cd to 6e5af94 Compare February 27, 2017 16:34

merge

bd5cd10

dreamofabear reviewed Feb 28, 2017

View reviewed changes

dreamofabear merged commit 91159e7 into ampproject:master Feb 28, 2017

kmh287 added a commit to kmh287/amphtml that referenced this pull request Feb 28, 2017

cleanup from pr comments on ampproject#7599

7d06c2d

kmh287 mentioned this pull request Feb 28, 2017

cleanup from pr comments on #7599 #7838

Merged

kmh287 added a commit that referenced this pull request Mar 1, 2017

cleanup from pr comments on #7599 (#7838)

526173f

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

cleanup from pr comments on ampproject#7599 (ampproject#7838)

5a1efeb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent BindEvaluator from returning Error objects to web worker #7599

Prevent BindEvaluator from returning Error objects to web worker #7599

kmh287 commented Feb 16, 2017

dreamofabear left a comment

jridgewell commented Feb 16, 2017

kmh287 commented Feb 17, 2017 •

edited

Loading

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017 •

edited

Loading

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017

dreamofabear Feb 21, 2017

dreamofabear Feb 21, 2017

dreamofabear Feb 21, 2017

kmh287 Feb 21, 2017 •

edited

Loading

dreamofabear Feb 21, 2017

dreamofabear Feb 22, 2017

kmh287 Feb 23, 2017

dreamofabear Feb 22, 2017

kmh287 Feb 23, 2017

dreamofabear Feb 22, 2017

kmh287 Feb 23, 2017

jridgewell Feb 23, 2017

dreamofabear Feb 28, 2017

dreamofabear Feb 28, 2017

dreamofabear Feb 28, 2017

dreamofabear commented Feb 28, 2017

Prevent BindEvaluator from returning Error objects to web worker #7599

Prevent BindEvaluator from returning Error objects to web worker #7599

Conversation

kmh287 commented Feb 16, 2017

dreamofabear left a comment

Choose a reason for hiding this comment

jridgewell commented Feb 16, 2017

kmh287 commented Feb 17, 2017 • edited Loading

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017 • edited Loading

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017

jridgewell commented Feb 17, 2017

kmh287 commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmh287 Feb 21, 2017 • 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

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

dreamofabear commented Feb 28, 2017

kmh287 commented Feb 17, 2017 •

edited

Loading

kmh287 commented Feb 17, 2017 •

edited

Loading

kmh287 Feb 21, 2017 •

edited

Loading