cleanup: Change type defs for state types to records

[BeksOmega]

The basics

• I branched from project-cereal
• My pull request is against project-cereal
• My code follows the style guide

Link for Diff: BeksOmega/blockly@cereal/plugin-hooks...BeksOmega:cereal/types

The details

Resolves

Work on project cereal
Dependent on #5276

Proposed Changes

Changes the type declarations for state types (ie block state and variable state) to use class declarations and @record annotations, as recommended by the styleguide. See 5.4.9 and the bottom of 7.7

Reason for Changes

Styleguide conformity. Plus it allows for more explicit docs.

Test Coverage

Not necessary in this case.

[maribethb]

Don't really think you need the |undefined since * means ALL which includes undefined.

[BeksOmega]

Weird, what you said makes sense so I removed it, but it gave me a compiler error. Maybe |undefined isn't required in general, but it is when specifying optional properties?

[maribethb]

Yeah, I guess that makes sense too.

[BeksOmega]

Ok so, this has run into a bit of an issue. Closure does not allow you to access properties of @structs using [''] notation. And it seems all @records are also @structs and there's no way to fix that.

I want to use an @record type so that we can provide detailed type information in the JSDoc and typescript types. But I also want to use [''] notation so that the closure compiler doesn't rename the properties (since they could be coming from JSON).

I'm really not sure what to do here. I did find one recommendation, which was to basically wrap any JSON you recieve in a new object with new rename-able properties. But I think that for that to work, we would have to change the parameter to the load function to be an !Object, which doesn't give us the benefits of the JSDoc or typescript types.

/**
 * @param {!Object} state // No benefit from types.
 */
const load = function(state, workspace) {
  const typedState = {
    // this property could be renamed by the compiler.
    // but it is still compatible with JSON
    id: state['id'],
    // etc...
  }
}

The only option I can think of is to not merge this, and stick with the workable, but less informative, typedefs. @maribethb thoughts?

[maribethb]

So, yes, I think for now the best bet is to stick with typedefs. (I wouldn't take the advice of that StackOverflow post without further research because it's from 2011 (!) and @struct/@dict annotations weren't added at all until 2012.) I was hoping you could just throw a @dict or an @unrestricted annotation onto the record, but apparently that's not the case :/

I think the actual correct thing to do in an ideal world is to have our external API defined in externs (here is where I would add a link to the internal JS Best Practices site) so that the compiler does not rename these properties. After all, these objects aren't really behaving like dictionaries. We just treat them that way as a convenient way to turn off property renaming by the compiler. But I don't think we currently supply any externs and I don't think you should have to figure that out as part of this project so I think going back to a typedef and using bracket access is fine and matches what we already have in the xml serializers.

[maribethb]

Yeah, I guess that makes sense too.

[BeksOmega]

Closing since records don't work with the compiler in this case and externs are hard.