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.

Sign up for GitHub

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

Disable additional .array() modifier #406

Merged
merged 6 commits into from
Dec 6, 2024
Merged

Disable additional .array() modifier #406

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b57589a
Disable additional .array() modifier
zxl629 Dec 2, 2024
6cd8bd8
Fix spacing
zxl629 Dec 2, 2024
f70ead7
Add unit test
zxl629 Dec 2, 2024
8c90469
Add defined-behavior test
zxl629 Dec 3, 2024
5f86338
Clean up docstring
zxl629 Dec 4, 2024
155fa09
Merge branch 'feat/reinvent-blocked-days/main' into lzhouq/fix/disall…
iartemiev Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/selfish-comics-kiss.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@aws-amplify/data-schema': minor
---

Disable additional .array() modifier on model field definition
3 changes: 2 additions & 1 deletion packages/data-schema/docs/data-schema.modelfield.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ Public API for the chainable builder methods exposed by Model Field. The type is
export type ModelField<T extends ModelFieldTypeParamOuter = ModelFieldTypeParamOuter, UsedMethod extends UsableModelFieldKey = never, Auth = undefined> = Omit<{
[__auth]?: Auth;
[brandSymbol]: typeof brandName;
[internal](): ModelField<T, UsedMethod>;
required(): ModelField<Required<T>, UsedMethod | 'required'>;
array(): ModelField<ArrayField<T>, Exclude<UsedMethod, 'required'>>;
array(): ModelField<ArrayField<T>, Exclude<UsedMethod, 'required'> | 'array'>;
default(value?: ModelFieldTypeParamOuter): ModelField<T, UsedMethod | 'default'>;
authorization<AuthRuleType extends Authorization<any, any, any>>(callback: (allow: Omit<AllowModifier, 'resource'>) => AuthRuleType | AuthRuleType[]): ModelField<T, UsedMethod | 'authorization', AuthRuleType>;
}, UsedMethod>;
Expand Down
14 changes: 12 additions & 2 deletions packages/data-schema/src/ModelField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const __auth = Symbol('__auth');
export const __generated = Symbol('__generated');

const brandName = 'modelField';
const internal = Symbol('internal');

export enum ModelFieldType {
Id = 'ID',
Expand Down Expand Up @@ -82,7 +83,7 @@ export type BaseModelField<

export type UsableModelFieldKey = satisfy<
methodKeyOf<ModelField>,
'required' | 'default' | 'authorization'
'required' | 'default' | 'authorization' | 'array'
>;

/**
Expand All @@ -102,6 +103,12 @@ export type ModelField<
[__auth]?: Auth;
[brandSymbol]: typeof brandName;

/**
* Internal non-omittable method to pass the test, won't be used by customers.
*/
[internal](): ModelField<T, UsedMethod>;
//TODO: should not be needed. Fix later
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. 😅

What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stocaaro stocaaro Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@iartemiev iartemiev Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:
image
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:
image
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.
image

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. I think this makes sense.

iartemiev marked this conversation as resolved.
Show resolved Hide resolved

/**
* Marks a field as required.
*/
Expand All @@ -110,7 +117,7 @@ export type ModelField<
/**
* Converts a field type definition to an array of the field type.
*/
array(): ModelField<ArrayField<T>, Exclude<UsedMethod, 'required'>>;
array(): ModelField<ArrayField<T>, Exclude<UsedMethod,'required'> | 'array'>;
// TODO: should be T, but .array breaks this constraint. Fix later
/**
* Sets a default value for the scalar type.
Expand Down Expand Up @@ -199,6 +206,9 @@ function _field<T extends ModelFieldTypeParamOuter>(fieldType: ModelFieldType) {
return this;
},
...brand(brandName),
[internal]() {
return this;
},
} as ModelField<T>;

// this double cast gives us a Subtyping Constraint i.e., hides `data` from the public API,
Expand Down
Loading