Disable additional .array() modifier

[zxl629]

Related issues:

• Creating/Reading a matrix is broken amplify-js#13936

Description of changes:

This change disables customers from chaining .array() modifier in model field definition. It adds .array() method to the used method list after being called.

Example of why we need this:

Currently customers can declare a double array for a data field, which will cause errors and is not what we intended.

MatrixTest: a.model({ values: a.string().required().array().required().array().required() })
.authorization((allow) =>[allow.publicApiKey()])

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[changeset-bot]

🦋 Changeset detected

Latest commit: 155fa09

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[svidgen]

In addition to addressing the comment on the internal symbol, let's also please add some testing. (Ideally in defined-behavior.)

Thanks!

[svidgen]

I don't understand. 😅

What am I missing?

[zxl629]

It seems that making every builder method omittable will break BaseModelField, as there is no method signature in BaseModelField type. Adding an internal method here solves the issue. However, this is not a clean solution and I am not sure how to work around it.

[stocaaro]

I think this is the same problem being solved elsewhere with branding. Would a brand work to distinguish this?

[svidgen]

OK. So, the fact that this is a method and not just a regular property is kind of incidental?

[zxl629]

There is a brand property in ModelField. I could be wrong here but the errors showed up in ModelField.test-d.ts suggest TypeScript cannot infer the correct type any more unless there is a method in place for ModelField.

[iartemiev]

@stocaaro - not the same problem. By making all methods omittable, we're breaking BaseModelField's ability to retain generic type information from ModelField. I'll try to illustrate below.

before this change:

Note: that there's nothing special about the array method, we just need at least one property on this type to reference ModelField's T type arg.

If we just omit array without adding another property:

This causes BaseModelField to lose the type information in T that was assigned or modified by the modifier methods on ModelField and the type argument defaults to ModelFieldTypeParamOuter. So any consumers of BaseModelField cannot infer any specific type metadata out of it, e.g. the underlying field type, nullability, "arrayness".

Adding another property that references T, keeps BaseModelField intact.

@svidgen

OK. So, the fact that this is a method and not just a regular property is kind of incidental?

Bingo. We just need some non-omittable property that maintains a reference to T. Simplest form would be:

{
  [internal]: T;
}

But that makes the implementation side a lot clunkier. I was only able to satisfy that type with a double assertion.

We can do e.g.

{
  [internal]: ModelType<T>;
}

And then something like

{
  ...,
  [internal]: _field(ModelFieldType.String)
}

In the implementation, but that still looks worse/more arbitrary than what we've got now with the method.

[stocaaro]

Ack. I think this makes sense.

[zxl629]

I have added a unit test in ModelField.test.ts file and a behavior test in 1-patterns/add-fields.ts file. I wonder if we need it in the defined behavior test since technically the errors shows up when user is writing the schema.

In addition to addressing the comment on the internal symbol, let's also please add some testing. (Ideally in defined-behavior.)