Questions for fixing up TypeScript tests

[DanielRosenwasser]

Hi there, I'm making some changes to correct the typings for Breeze on DefinitelyTyped/your repo as part of microsoft/TypeScript#4081.

I've run into two issues that I want to make sure I'm correcting...correctly 😄

    var entityManager = new breeze.EntityType({
        metadataStore: myMetadataStore,
        serviceName: "breeze/NorthwindIBModel",
        name: "person",
        namespace: "myAppNamespace"
    });

In TypeScript 1.6, we give an error stating that metadataStore isn't an expected property in EntityTypeOptions. I looked into the documentation, and indeed it seems that it takes either EntityTypeOptions or a MetadataStore type. Can you confirm that this is correct?

2. In one example, we have

var v0 = breeze.Validator.maxLength({ maxLength: 5, displayName: "City" });

Is displayName an expected property here? Is it a name that's dynamically used at runtime? Or did the user meanmessageTemplate`?

Sorry to ask these (probably simple) questions - I'm not entirely familiar with the project, but I'd like to help the best I can.

[wardbell]

I will look into this as soon as I get my computer back

[DanielRosenwasser]

Hey @wardbell, any update on the situation?

[wardbell]

(1) Yes the EntityType ctor takes EITHER a MetadataStore OR an EntityTypeOptions. I'm surprised that there is a problem as the breeze.d.ts on definitely typed shows both constructors.

constructor(config: MetadataStore);
constructor(config: EntityTypeOptions);

These days I think you could write this as

constructor(config: MetadataStore | EntityTypeOptions);

That wasn't an option when this d.ts was first composed.

If we were providing comments (as I think we should) the two-line overload form is actually the better choice:

/**
 Create an EntityType with a {#crossLink "MetadataStore"}.  Defines an 'anonymous' type that will never be
 communicated to or from the server. It is purely for client side use and will be given an
 automatically generated name.
 **/
constructor(config: EntityTypeOptions);

/**
 Create an EntityType with an {#crossLink "EntityTypeOptions"} configuration object.
**/
constructor(config: MetadataStore);

I guess I should note that the d.ts definition of the EntityTypeOptions interface is out-of-date. It is currently:

interface EntityTypeOptions {
    shortName?: string;
    namespace?: string;
    autoGeneratedKeyType?: AutoGeneratedKeyType;
    defaultResourceName?: string;
    dataProperties?: DataProperty[];
    navigationProperties?: NavigationProperty[];
}

It should be:

interface EntityTypeOptions {
    shortName?: string;
    namespace?: string;
    baseTypeName?: string;
    isAbstract?: Boolean;
    autoGeneratedKeyType?: AutoGeneratedKeyType;
    defaultResourceName?: string;
    dataProperties?: DataProperty[];
    navigationProperties?: NavigationProperty[];
    serializerFn?: (dp:DataProperty, value:any) => any;
}

I don't know how many of these we have or who has time to fix them all. If/when we re-write Breeze in TS, we'll be able to get these right and maintain them.

---

(2) The displayName in breeze.Validator.maxLength is intentional and not to be confused with messageTemplate which is a different, valid property name in a "context" object.

The displayName is the name of a token within this validator's default message template. When passed as a parameter (as in your example), the value is interpolated into that message template.

This is a general pattern for validators in which the property value of the validator's "context" object parameter is interpolated into the generated message when the property key matches a token in the message template.

The actual message template used will be the default defined for that validator UNLESS the optional messageTemplate property on that "context" object parameter has been specified (in which case its value is the effective template).

TS cannot validate all of the possible property keys that might match a token in a message template ... because that is unknowable. The developer could change the effective message template at run-time as in this nonsense example:

maxLength({ 
  maxLength: 5,
  foo: 'Foo', 
  baz: 42,
  displayName: 'City', 
  messageTemplate: 'Put your %foo% before your %baz%!'
})

What we know from reading the source code is that Breeze will interpolate the context property's value if it's in the message template and will ignore that value if the property key is not found as a token in the message template. In the example above with custom template, foo and baz are used; displayName is ignored.

The .d.ts author wrote this declaration for maxLength

static maxLength(context: { maxLength: number; messageTemplate?: string }): Validator;

There is a mistake here in that the "context" parameter's two properties should be separated by a comma, not a semicolon which is illegal .Apparently this mistake is harmless (which surprises me). Or maybe the error drew your attention.

Looking past this, what could the author have done?

The minimal acceptable definition is

 static maxLength(context: { maxLength: number }): Validator;

We have to state that maxLength is required because the validator won't work without it. But all other context keys are optional. Why mention any at all? My answer: for intellisense.

Breeze looks for the messageTemplate property specifically. We should tell the developer about that. Which is why maxLength should be defined as:

 static maxLength(context: { maxLength: number, messageTemplate?: string }): Validator;

I might argue that we should also include the tokens in the default message template. displayName is one such a token so it would have been acceptable to write:

static maxLength(context: { 
  maxLength: number,
  messageTemplate?: string, 
  displayName?: string }): Validator;

That would give even better intellisense. But, as I said, from a typechecking perspective, only the maxLength is required.

HTH.

[DanielRosenwasser]

It sounds like these validators will need an appropriate index signature to any. I'm not sure if/when I'll get the chance to buckle down and fix these, but a quick fix you could do is:

type TemplateProperties = { [prop: string]: any; };

class Validator {
    static maxLength(context: { maxLength: number, messageTemplate?: string } & TemplateProperties): Validator;
}

[wardbell]

Is that the new v.1.6 strict object literal checking stuff in action?

I really like the v.1.6 changes, including the intersection operator, &, as used here.

But what will these changes do to someone who has not upgraded to v.1.6 yet? Would the v.1.5 compiler/intellisense be stumped by the new stuff like the intersection operator?

And the questions keep coming ;-)

Does your definition of TemplateProperties really help me that much relative to ...

class Validator {
  static maxLength(context: { maxLength: number, messageTemplate?: string, [prop: string]: any }): Validator;
}

Or, if I'm going to use TemplateProperties for all static validators and they all have the properties messageTemplate?: string and displayName?:string, should I go with this?

type TemplateProperties = { displayName?: string, messageTemplate?: string,  [prop: string]: any };

class Validator {
  static maxLength(context: { maxLength: number} & TemplateProperties): Validator;
}

[DanielRosenwasser]

But what will these changes do to someone who has not upgraded to v.1.6 yet? Would the v.1.5 compiler/intellisense be stumped by the new stuff like the intersection operator?

Correct; if you don't want to use the intersection operator, you can easily just add the index signature to each type literal like you did above (or create a TemplateProperties interface, and an interface for each validator parameter, each which extendsTemplateProperties`). That way you can keep compatibility with your 1.5 users as well.

And yes, you could move messageTemplate to TemplateProperties now that you mention it - I trust you probably know better than me on this. 😄