Eliminate the need to cast from Object to JsonObject

[rsimha]

This came out of the discussions at #23559 (comment) and #23559 (comment)

The AMP codebase contains hundreds of casts from Object to JsonObject.

• I wonder if there's a way to modify the declaration of JsonObject in amp.extern.js to make most of the casts unnecessary
  

  amphtml/build-system/amp.extern.js

  Lines 57 to 65 in 6b86000

  	/**
  	* A type for Objects that can be JSON serialized or that come from
  	* JSON serialization. Requires the objects fields to be accessed with
  	* bracket notation object['name'] to make sure the fields do not get
  	* obfuscated.
  	* @constructor
  	* @dict
  	*/
  	function JsonObject() {}

• For converting to JsonObject, there is parseJson() from src/json.js, but it doesn't work for cases where a non-null JsonObject is expected
  

  amphtml/src/json.js

  Lines 104 to 113 in c4a663d

  	/**
  	* Simple wrapper around JSON.parse that casts the return value
  	* to JsonObject.
  	* Create a new wrapper if an array return value is desired.
  	* @param {*} json JSON string to parse
  	* @return {?JsonObject} May be extend to parse arrays.
  	*/
  	export function parseJson(json) {
  	return /** @type {?JsonObject} */ (JSON.parse(/** @type {string} */ (json)));
  	}

• For checking if a variable is an Object, there is isObject() from src/types.js, but it doesn't work when a JsonObject is expected
  

  amphtml/src/types.js

  Lines 48 to 55 in c4a663d

  	/**
  	* Determines if value is actually an Object.
  	* @param {*} value
  	* @return {boolean}
  	*/
  	export function isObject(value) {
  	return toString(value) === '[object Object]';
  	}

Related to #23425

[rsimha]

Tagging @ampproject/wg-runtime @ampproject/wg-infra @ampproject/wg-performance for visibility.

[rsimha]

@jridgewell @erwinmombay Do you know of a way to provide an implicit conversion from Object to JsonObject?

For example, this code will currently fail gulp check-types:

  /**
   * @return {!JsonObject}
   */
  foo() {
    return Object.assign({}, {'foo': 'bar'});
  }

ERROR - [JSC_TYPE_MISMATCH] inconsistent return type
found   : Object
required: JsonObject
    return Object.assign({}, {'foo': 'bar'});
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

However, adding a cast fixes it:

  /**
   * @return {!JsonObject}
   */
  foo() {
    return /** @type {!JsonObject} */ (Object.assign({}, {'foo': 'bar'}));
  }

[jridgewell]

Cast is the only way.

[kristoferbaxter]

Agree with @jridgewell, these are cases where casting makes sense.

[erwinmombay]

we also have dict which is a helper function that does this but the extra call might not be worth it (im not sure if it is completely optimized and erased in all cases for closure since it is a pass through call). maybe in these cases there needs to be a dictAssign helper since the problem here is that Object.assign returns Object

export function dict(opt_initial) {
  // We do not copy. The linter enforces that the passed in object is a literal
  // and thus the caller cannot have a reference to it.
  return /** @type {!JsonObject} */ (opt_initial || {});
}

[rsimha]

@jeffkaufman, you recently pointed out casts that seemed unnecessary in ads code. Would dict() help in those cases?

[dreamofabear]

Stepping back a bit, are we sure we want to use JsonObject for this use case instead of an extern? You avoid unintentional obfuscation but you lose property typing.

IMO we should extern structs whose shape we definitively know. And only use JsonObject for truly arbitrary data e.g. objects that are actually parsed from JSON.

[rsimha]

Stepping back a bit, are we sure we want to use JsonObject for this use case instead of an extern? You avoid unintentional obfuscation but you lose property typing.

Good question. The JsonObject type was added by @cramforce in #9875 (link) as a way to prevent property obfuscation.

IMO we should extern structs whose shape we definitively know. And only use JsonObject for truly arbitrary data e.g. objects that are actually parsed from JSON.

Today, we have about ~1000 instances of JsonObject in our code, more than 200 of which are in @param annotations. Not sure how many of these can be converted to known structs.

I believe @erwinmombay is working on a way to automatically extract known structs so they can be declared in an externs file and used in place of JsonObject.

[rsimha]

Assigning to @erwinmombay, who is dealing with this in #24453

[rsimha]

@erwinmombay Looks like #24453 has been closed as stale without merging it. Is this still a worthwhile undertaking?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[danielrozenberg]

@rileyajones you've been doing a lot of typing-related work recently, does this issue still make sense?

[rileyajones]

I think this issue no longer warrants a ticket as the move to TS typing should resolve the underlying issue

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.