♻️ Fixit: standalone assert helpers w/ format messages #32788

rcebulko · 2021-02-22T16:59:50Z

This PR:

adds support for format string assertion messages (devAssert('Failed to %s', foobar))
adds tests for pure-assert.js
modifies uses of pureDevAssert/pureUserAssert to import as to make it easy to remove "pure" from name once split is complete

rcebulko · 2021-02-22T17:12:02Z

src/pure-assert.js

-  return shouldBeTruthy;
+
+  // Substitute provided values into format string in message
+  const message = Array.prototype.slice


There is one thing this does not preserve: logic which detects if an element is passed and transforms the resulting error message to contain extra element info, adds associatedElement property, adds fromAssert property, etc. This felt AMP-centric and not necessarily needed for a standalone helper. Can add back in now or down the line if deemed necessary

Compare to https://github.com/ampproject/amphtml/blob/master/src/log.js#L389

That might actually have significant value for useAssert. But definitely has zero value for devAssert. Another reason why they are not all that similar.

src/pure-assert.js

caroqliu · 2021-02-22T18:11:54Z

src/pure-assert.js

+ * @param {*=} opt_6 Optional argument
+ * @param {*=} opt_7 Optional argument
+ * @param {*=} opt_8 Optional argument
+ * @param {*=} opt_9 Optional argument


Is there a reason there are exactly 9 optional arguments? Could this be a {...*} like for pureAssertion above?

Per JSDoc: var args as individual params for better inlining at compile-time

caroqliu · 2021-02-22T18:13:26Z

src/utils/date.js

@@ -57,9 +60,9 @@ export function getDate(value) {
 /** Map from attribute names to their parsers. */
 const dateAttrParsers = {
  'datetime': (datetime) =>
-    pureUserAssert(parseDate(datetime), `Invalid date: ${datetime}`),
+    userAssert(parseDate(datetime), 'Invalid date: %s', datetime),


FMI: Why is this format preferred to template literals?

There is partial work (according to @erwinmombay it's mostly complete) around having Babel remove all assertion strings, and this would end up generating a URL to an error page, something like https://cdn.ampproject.org/error/1234?v1=${datetime}. Error strings themselves get compiled out and replaced with error codes, saving some bytes along the way. I don't know where the I2I or docs around this are at the moment.

That sort of makes sense. But another reason why this is useful: we avoid constructing the error string if userAssert does not throw an error. And we expect that throwing an error here would be an exception (pun semi-intended).

I believe we supported both forms, you just couldn't mix %s inside a template literal. @alanorozco

caroqliu · 2021-02-22T18:17:35Z

test/unit/test-pure-assert.js

+    });
+
+    it('should fail for falsey values dev', () => {
+      expect(() => userAssert(false, 'xyz')).to.throw(


Do these not need to be wrapped with allowConsoleError?

As written, the assert helpers don't console log, they just throw the error. It wasn't clear to me that it was logged before, but inspecting further I see it is. console.assert() could easily be used here, but do we think this is still necessary if the assertion throws an errer?

Since this already throws an error, that error is either a) unhandled, and reaches console anyway or b) caught and handled, in which case it would make sense for the catching code to handle/optionally log or swallow the error. Thoughts?

src/pure-assert.js

dvoytenko · 2021-02-22T18:40:13Z

src/pure-assert.js

@@ -55,40 +55,118 @@ export class UserError extends Error {
 * Throws a provided error if the second argument isn't trueish.
 * @param {Object} errorCls
 * @param {T} shouldBeTruthy
- * @param {string} message
+ * @param {string} opt_message


I'd just name the file src/assert.js. Another consideration here: I'd also consider the following separation that might work better for us:

dev.js - all dev asserts and other utils. This whole module is completely stripped from the PROD - nothing remains at all. Easy to check that this module is not part of any compilation.

user.js - all user-related utils to assert/log.

I guess what I'm really wondering here: why try to keep devAssert and userAssert together. Besides the fact that they both throw exceptions, is there anything else that they share? Another benefit: if we focus on devAssert we might be able to get through all of them quicker then userAssert and co.

/cc @jridgewell

re: renaming, I was planning to do that in a follow-up, but it probably makes sense to do sooner rather than later. Will add here

I'd just name the file src/assert.js
Done

re: alternate split, it's not clear to me how much dev.js/user.js would own. I'd almost rather see something like a closure compiler comment/directive like @devOnly that lets helpers live in the appropriate module with related logic. Curious to hear Justin's thoughts

The critical part is devAssert is basically covering for cases that compiler is not sufficient. When compiler has done its job, the devAsserts should disappear as they do now. I think this is a lot better than @devOnly since it's still a real code, just not something we want in the prod binary.

dvoytenko · 2021-02-22T18:42:47Z

src/pure-assert.js

+      opt_message || 'Assertion failed'
+    );
+
+  throw new errorCls(message);


Just to clarify: having a special UserError class means relatively little because the errors are caught and rethrown all the time, sometimes by the code that we do not control, so the message testing is the only fully reliable way that we've found.

Ack; expectation here was Bento code may throw UserError directly if we do not re-create user.error() structure. Will keep an eye on how this is used as error handling comes into place

This is absolutely ok to throw a UserError by our code. Just making sure we're not overly reliant on it downstream. E.g. for a reporter !(error instanceof UserError) might still be a user error because someone has rethrown it weirdly.

dvoytenko · 2021-02-22T18:47:21Z

src/pure-assert.js

 * @closurePrimitive {asserts.truthy}
 */
-export function pureDevAssert(shouldBeTruthy, message) {
-  return pureAssertion(Error, shouldBeTruthy, message);
+export function pureDevAssert(


IMHO we should just export devAssert name. Also, let's try to replace all instances of devAssert with this one ASAP. Doesn't have to be this PR, but ideally this week we could switch completely to this version.

Plan (largely implemented in a handful of local branches already):

make signatures match versions from log.js

update code to import {pureDevAssert as devAssert} etc in a handful of PRs

remove versions from log.js, rename removing "pure" part, find-replace all references

With the goal of making the latter two steps less likely to complicate merges during Fixit

Any number of steps is ok. As long as we have an overall plan.

dvoytenko · 2021-02-22T18:48:05Z

src/pure-assert.js

 }

 /**
 * Throws an error if the first argument isn't trueish. Mirrors devAssert in
 * src/log.js.
 * @param {T} shouldBeTruthy
- * @param {string} message
+ * @param {string} opt_message


We should also move devAssertElement here - it's super common and useful. And current dev().assertElement really bugs me.

I agree; would like to do this down the line if that's alright, after the two main dev/userAssert helpers are done

src/pure-assert.js

dvoytenko · 2021-02-22T18:50:41Z

src/pure-assert.js

-  return shouldBeTruthy;
+
+  // Substitute provided values into format string in message
+  const message = Array.prototype.slice


That might actually have significant value for useAssert. But definitely has zero value for devAssert. Another reason why they are not all that similar.

dvoytenko · 2021-02-22T18:54:34Z

src/utils/date.js

@@ -57,9 +60,9 @@ export function getDate(value) {
 /** Map from attribute names to their parsers. */
 const dateAttrParsers = {
  'datetime': (datetime) =>
-    pureUserAssert(parseDate(datetime), `Invalid date: ${datetime}`),
+    userAssert(parseDate(datetime), 'Invalid date: %s', datetime),


That sort of makes sense. But another reason why this is useful: we avoid constructing the error string if userAssert does not throw an error. And we expect that throwing an error here would be an exception (pun semi-intended).

jridgewell · 2021-02-22T20:30:53Z

src/assert.js

+  }
+
+  // Substitute provided values into format string in message
+  const message = Array.prototype.slice


Nit: Is this needed if they just did a template literal in as the message?

See #32788 (comment)

caroqliu · 2021-02-23T15:36:31Z

extensions/amp-selector/1.0/base-element.js

@@ -96,7 +96,7 @@ function getOptions(element, mu) {
    .filter(
      (el) =>
        !closestAncestorElementBySelector(
-          pureDevAssert(
+          devAssert(
            el.parentElement?.nodeType == 1 && el.parentElement,


Suggested change

el.parentElement?.nodeType == 1 && el.parentElement,

el.parentElement && el.parentElement.nodeType == 1,

Not crucial for this PR: Is it me or is it kind of funny this is in this order?

Agreed, very weird! This was introduced by yours truly in #32474 (see original assertion) when rewriting an assertElement. Nice catch! dropped the el.parentElement altogether since el.parentElement?.nodeType == 1 captures it all.

test/unit/test-assert.js

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

rcebulko requested review from dvoytenko and caroqliu February 22, 2021 16:59

rcebulko commented Feb 22, 2021

View reviewed changes

caroqliu reviewed Feb 22, 2021

View reviewed changes

dvoytenko suggested changes Feb 22, 2021

View reviewed changes

jridgewell approved these changes Feb 22, 2021

View reviewed changes

rcebulko requested review from caroqliu and dvoytenko February 22, 2021 20:58

rcebulko added 6 commits February 22, 2021 16:04

support formattrings in asserts

53ef393

Add tests for pure-assert

01f1804

Import as pure asserts

4f7fcc1

Update date.js to use format args

a1c1d9a

rename pureAssertion to assertion

3637025

Rename pure-assert.js to assert.js

d05b755

rcebulko force-pushed the assert-vararg branch from 5681b99 to d05b755 Compare February 22, 2021 21:05

Fix test file

5cfcbbd

caroqliu approved these changes Feb 23, 2021

View reviewed changes

rcebulko and others added 3 commits February 23, 2021 10:50

Fix copyright year

3525a00

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

fix assertion

beb9754

Lint fixes

30e79e6

dvoytenko approved these changes Feb 23, 2021

View reviewed changes

rcebulko merged commit 8c31741 into ampproject:master Feb 23, 2021

rcebulko deleted the assert-vararg branch February 23, 2021 17:52

rcebulko mentioned this pull request Feb 23, 2021

♻️ Fixit: Update some imports of assertions to use standalone helpers #32828

Merged

rcebulko mentioned this pull request Mar 3, 2021

♻️ Update mixed imports of assertions to use standalone helpers #33036

Closed

ampprojectbot added the PR use: In Beta / Experimental label Mar 3, 2021

This was referenced Mar 4, 2021

🚨 Error: Assertion failed ampproject/error-reporting#60

Open

🚨 Error: Assertion failed ampproject/error-reporting#63

Open

🚨 Error: Assertion failed ampproject/error-reporting#64

Open

ampprojectbot added the PR use: In Stable label Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Fixit: standalone assert helpers w/ format messages #32788

♻️ Fixit: standalone assert helpers w/ format messages #32788

rcebulko commented Feb 22, 2021

rcebulko Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 22, 2021

caroqliu Feb 22, 2021

rcebulko Feb 22, 2021

caroqliu Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 22, 2021

jridgewell Feb 22, 2021 •

edited

caroqliu Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 22, 2021

rcebulko Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 23, 2021

dvoytenko Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 23, 2021

dvoytenko Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 23, 2021

dvoytenko Feb 22, 2021

rcebulko Feb 22, 2021

dvoytenko Feb 22, 2021

dvoytenko Feb 22, 2021

jridgewell Feb 22, 2021

rcebulko Feb 22, 2021

caroqliu Feb 23, 2021

rcebulko Feb 23, 2021

	el.parentElement?.nodeType == 1 && el.parentElement,
	el.parentElement && el.parentElement.nodeType == 1,

♻️ Fixit: standalone assert helpers w/ format messages #32788

♻️ Fixit: standalone assert helpers w/ format messages #32788

Conversation

rcebulko commented Feb 22, 2021

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

jridgewell Feb 22, 2021 • edited

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

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

jridgewell Feb 22, 2021 •

edited