Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate the need to cast from Object to JsonObject #23567

Open
rsimha opened this issue Jul 29, 2019 · 14 comments
Open

Eliminate the need to cast from Object to JsonObject #23567

rsimha opened this issue Jul 29, 2019 · 14 comments
Assignees
Labels
Stale Inactive for one year or more Type: Bug WG: performance

Comments

@rsimha
Copy link
Contributor

rsimha commented Jul 29, 2019

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

    /**
    * 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
Copy link
Contributor Author

rsimha commented Jul 29, 2019

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

@rsimha rsimha changed the title Remove the need to always cast from Object to JsonObject Eliminate the need to cast from Object to JsonObject Jul 29, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Jul 30, 2019

@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
Copy link
Contributor

Cast is the only way.

@kristoferbaxter
Copy link
Contributor

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

@erwinmombay
Copy link
Member

erwinmombay commented Jul 30, 2019

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
Copy link
Contributor Author

rsimha commented Jul 30, 2019

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

@dreamofabear
Copy link

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
Copy link
Contributor Author

rsimha commented Jul 30, 2019

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 rsimha self-assigned this Aug 14, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Sep 11, 2019

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

@rsimha
Copy link
Contributor Author

rsimha commented May 1, 2020

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

@stale
Copy link

stale bot commented Oct 24, 2021

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.

@stale stale bot added the Stale Inactive for one year or more label Oct 24, 2021
@danielrozenberg
Copy link
Member

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

@stale stale bot removed the Stale Inactive for one year or more label Oct 25, 2021
@rileyajones
Copy link
Contributor

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

@stale
Copy link

stale bot commented Oct 19, 2022

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.

@stale stale bot added the Stale Inactive for one year or more label Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more Type: Bug WG: performance
Projects
None yet
Development

No branches or pull requests

7 participants