Conversation
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Pull Request Test Coverage Report for Build 1269225996
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1270325123Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
types/connector.d.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _models?: Record<string, ModelBaseClass>; |
There was a problem hiding this comment.
Not sure if this is correct.
There was a problem hiding this comment.
Most probably ModelDefinition.
| export declare class DataSource extends EventEmitter { | ||
| name: string; | ||
| settings: Options; | ||
| settings: ConnectorSettings; |
There was a problem hiding this comment.
Based on the codebase, the DataSource simply passes the whole settings to the connector; Hence why the type called ConnectorSettings.
There was a problem hiding this comment.
On second thought, we may want to create a dedicated passthrough type to avoid potential future issues due to the shared type.
| // Reason for deprecation is not clear. | ||
| /** | ||
| * {@inheritDoc Connector.getTypes} | ||
| * @deprecated | ||
| */ | ||
| getTypes(): string[]; | ||
|
|
||
| /** | ||
| * Check if the datasource supports the specified types. | ||
| * @param types Type name(s) to check against | ||
| */ | ||
| supportTypes(types: string | string[]): boolean; |
There was a problem hiding this comment.
Based on the original codebase comments, getTypes() is deprecated but not supportTypes(); Not sure of the reason for this. The original purpose of these functions are also not clear.
| * @param propertyName Target property name | ||
| * @returns Column metadata | ||
| */ | ||
| columnMetadata(modelName: string, propertyName: string): ColumnMetadata; |
There was a problem hiding this comment.
This seems to be the only function that uses ColumnMetadata. However, ColumnMetadata typedef is non-specific. Need to find examples of the return value.
| * @param modelName Target model name | ||
| * @returns The ID property definition | ||
| */ | ||
| idProperty(modelName: string): PropertyDefinition; |
There was a problem hiding this comment.
There's also a IdDefinition in model.d.ts. Not sure if this function is supposed to return that instead.
| freezeDataSource?(): void; | ||
| freezeSchema?(): void; |
There was a problem hiding this comment.
Not sure the difference between these functions. DataSource.prototype.freeze calls both functions if they are defined.
There was a problem hiding this comment.
freezeDatasource seems to have been introduced by this commit:
9b169ef#diff-7de38f47065ef67b4936fbd6396bb71a580fe9a5a9ff2c9723c4fe859d1e2a6eR977-R979
freezeSchema was from JugglingDB.
There was a problem hiding this comment.
Based on the commit just before the one mentioned above, they should be identical, and the use of both is for backwards-compatibility with JugglingDB:
| // #TODO(achrinza): The codebase suggets that `context` differs | ||
| // depending on the situation, and that there's no cohesive interface. | ||
| // Hence, we'll need to identify all the possible contexts. | ||
| export type Context = { | ||
| Model: ModelBaseClass, | ||
| instance?: object, | ||
| query?: Filter, | ||
| where?: Where, | ||
| data?: AnyObject, | ||
| hookState?: AnyObject, | ||
| options?: Options, | ||
| isNewInstance?: boolean, | ||
| currentInstance?: Readonly<ModelBase>, | ||
| } |
There was a problem hiding this comment.
These are defined here:
- https://loopback.io/doc/en/lb3/Operation-hooks.html
- https://loopback.io/doc/en/lb3/Remote-hooks.html#context-object
- https://loopback.io/doc/en/lb3/Connector-hooks.html#context
Currently, this is only based on all the possible attributes for an Operation Hooks context.
Will probably need to add separate context typedefs for the other 2.
While the Operation Hooks context differs depending on the event being observed and the function called, I'm not sure if its better to create distinctive op. hook contexts for these different hooks.
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
|
|
||
| defineOperation(name: string, options: OperationOptions, fn: Function): void; | ||
|
|
||
| enableRemote(operation: string): void; |
There was a problem hiding this comment.
Should deprecate as an LB3-only feature.
|
|
||
| enableRemote(operation: string): void; | ||
|
|
||
| disableRemote(operation: string): void; |
There was a problem hiding this comment.
Should deprecate as an LB3-only feature.
| * {@link Connector.discoverModelProperties} is used instead. Otherwise, an | ||
| * error is thrown. |
There was a problem hiding this comment.
#TODO: What error is thrown?
| /** | ||
| * A hash-map of the different relation types. | ||
| */ |
There was a problem hiding this comment.
Let's include an example since it's not expected to change.
| initialized?: boolean; | ||
| connected?: boolean; | ||
| connecting?: boolean; |
There was a problem hiding this comment.
Add tsdoc explaining the circumstances for the possible values.
types/connector.d.ts
Outdated
| * | ||
| * @internal | ||
| */ | ||
| export declare class ConnectorBase implements Connector { |
There was a problem hiding this comment.
May need to extract this to loopback-connector package along with transaction-related typedefs.
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
| /** | ||
| * Opt-out unless stated otherwise | ||
| */ | ||
| export interface ConnectorCapabilities { |
There was a problem hiding this comment.
Separate between DAO and KVAO connector capabilities.
types/model.d.ts
Outdated
| excludeBaseProperties?: string[]; | ||
|
|
||
| /** | ||
| * Indicates if the {@link ModelBaseClass | Model} is attached to the DataS |
| * Alias of {@link ModelSettings.base}. Takes lower precedence. | ||
| */ | ||
| super?: ModelBaseClass; | ||
| excludeBaseProperties?: string[]; |
| * Model properties to be set as hidden. | ||
| * | ||
| * @remarks | ||
| * Hidden properties are |
types/model.d.ts
Outdated
| plural?: string; | ||
|
|
||
| http?: { | ||
| path?: string; | ||
| } |
There was a problem hiding this comment.
Deprecate; Add tsdoc explaining their purpose, defaults, and relationship between each other (e.g. plural is used for http.path when the latter is not set)
types/model.d.ts
Outdated
| // Postgresql-specific | ||
| type: string; | ||
| kind: string; |
| maxDepthOfQuery?: number; | ||
| maxDepthOfData?: number; | ||
| prohibitHiddenPropertiesInQuery?: boolean; |
There was a problem hiding this comment.
Explain these options with tsdoc.
| // #TODO(achrinza): The codebase suggets that `context` differs | ||
| // depending on the situation, and that there's no cohesive interface. | ||
| // Hence, we'll need to identify all the possible contexts. | ||
| export interface Context { | ||
| Model: ModelBaseClass; | ||
| instance?: object; | ||
| query?: Filter; | ||
| where?: Where; | ||
| data?: AnyObject; | ||
| hookState: object; | ||
| options: Options; | ||
| isNewInstance?: boolean; | ||
| currentInstance?: Readonly<ModelBase>; | ||
| } |
There was a problem hiding this comment.
#TODO(achrinza): The codebase suggets that context differs
depending on the situation, and that there's no cohesive interface.
Hence, we'll need to identify all the possible contexts.
types/datasource.d.ts
Outdated
| export type OperationOptions = { | ||
| accepts: string[], | ||
| returns: string[], | ||
| http: object, | ||
| remoteEnabled: boolean, | ||
| scope: unknown, | ||
| fnName: string, | ||
| } |
There was a problem hiding this comment.
This looks like it's for "HTTP remoting"; We should mark as deprecated.
EDIT 1: Not deprecated
EDIT 2: May be deprecated even though used internally - Need to re-review.
| buildModelFromInstance( | ||
| modelName: string, | ||
| jsonObject: AnyObject, | ||
| options?: Options, | ||
| ): ModelBaseClass; |
There was a problem hiding this comment.
Add documentation on what (Connector?) methods this calls behind the scenes.
| export declare class DataSource extends EventEmitter { | ||
| name: string; | ||
| settings: Options; | ||
| settings: ConnectorSettings; |
There was a problem hiding this comment.
On second thought, we may want to create a dedicated passthrough type to avoid potential future issues due to the shared type.
types/connector.d.ts
Outdated
| * @deprecated Use {@link ConnectorSettings.connector} instead. | ||
| */ | ||
| adapter?: ConnectorStatic | string; | ||
| database: string; |
There was a problem hiding this comment.
Add tsdoc explaining that this is used by the Connectors directly.
| @@ -219,7 +405,11 @@ export declare class ModelBase { | |||
| * If true, then protected properties should not be brought out. | |||
| * @returns {object} returns Plain JSON object | |||
types/model.d.ts
Outdated
| * Comma-separated column names | ||
| * | ||
| * @remarks | ||
| * Handled by {@link Connector}s directly by default. |
There was a problem hiding this comment.
What does "by default" mean? What is the circumstance where this is not the case, and what is the impact?
types/model.d.ts
Outdated
| * Array of column names to create an index. | ||
| * | ||
| * @remarks | ||
| * Handled by {@link Connector}s directly by default. |
There was a problem hiding this comment.
Ditto. What does "by default" mean? What is the circumstance where this is not the case, and what is the impact?
types/connector.d.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _models?: Record<string, ModelBaseClass>; |
There was a problem hiding this comment.
Most probably ModelDefinition.
types/datasource.d.ts
Outdated
| models: Record<string, typeof ModelBase>; | ||
|
|
||
| definitions: {[modelName: string]: ModelDefinition}; |
There was a problem hiding this comment.
Will need to check and document these' relationship with Connector._models
There was a problem hiding this comment.
These are populated from the same-named properties of the ModelBuilder instance passed during class initialisation.
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
|
JugglingDB documentation: https://1602.github.io/jugglingdb/#DOCUMENTATION |
| * @remarks | ||
| * Alias of {@link ModelSettings.base}. Takes lower precedence. | ||
| */ | ||
| super?: ModelBaseClass; |
There was a problem hiding this comment.
todo: Document that this convention is from node:util.inherits.
types/connector.d.ts
Outdated
| * @param options Discovery options | ||
| * @param cb Callback function | ||
| */ | ||
| discoverSchemas?(tableName: string, options: SchemaDiscoveryOptions, cb: Callback<Schema>): Promise<Schema>; |
There was a problem hiding this comment.
clarification: Are the 2nd and 3rd parameters supposed to be optional? Check the return type as well.
| discoverSchemas?(tableName: string, options: SchemaDiscoveryOptions, cb: Callback<Schema>): Promise<Schema>; | |
| discoverSchemas?(tableName: string, options?: SchemaDiscoveryOptions, cb?: Callback<Schema>): Promise<Schema>; |
types/datasource.d.ts
Outdated
| constructor(name: string, settings?: ConnectorSettings, modelBuilder?: ModelBuilder); | ||
|
|
||
| constructor(settings?: ConnectorSettings, modelBuilder?: ModelBuilder); | ||
|
|
||
| constructor( | ||
| connectorModule: Connector, | ||
| settings?: Options, | ||
| settings?: Omit<ConnectorSettings, 'adapter' | 'connector'>, | ||
| modelBuilder?: ModelBuilder, | ||
| ); |
There was a problem hiding this comment.
issue: This should be more lenient. the first parameter should be connectorAndDataSourceName, with the 2nd parameter's connector/adapter and name taking precedence if they exist.
| /** | ||
| * Sets if JavaScript {@link undefined} as an attribute value should be | ||
| * persisted as database `NULL`. | ||
| */ | ||
| persistUndefinedAsNull?: boolean; |
There was a problem hiding this comment.
What's the default value?
| * - {@link DataAccessObject.removeById}/{@link DataAccessObject.destroyById}/{@link DataAccessObject.deleteById} | ||
| * - {@link DataAccessObject.remove}/{@link DataAccessObject.delete}/{@link DataAccessObject.destroy} | ||
| */ | ||
| strictDelete?: boolean; |
There was a problem hiding this comment.
What's the default value? IIRC, it's true as LoopBack 4's DefaultCrudRepository explicitly sets this to false:
| import {DataSource} from './datasource'; | ||
| import {Listener, OperationHookContext} from './observer-mixin'; | ||
| import {ModelUtilsOptions} from './model-utils'; | ||
|
|
There was a problem hiding this comment.
interface Schema has an options object property?
| patch: boolean; | ||
| }; // Only referenced in "legacy built-in merge policy" | ||
| options?: { | ||
| path: boolean; |
There was a problem hiding this comment.
This looks like a typo
| path: boolean; | |
| patch: boolean; |
types/model.d.ts
Outdated
| /** | ||
| * {@inheritDoc ModelSettings.tableName} | ||
| */ | ||
| tableName?: string | ||
|
|
||
| /** | ||
| * Mapped table name for the model. | ||
| */ | ||
| table?: string; |
There was a problem hiding this comment.
Which one takes precedence?
types/datasource.d.ts
Outdated
| accepts: string[], | ||
| returns: string[], | ||
| http: object, | ||
| remoteEnabled: boolean, | ||
| scope: unknown, | ||
| fnName: string, |
There was a problem hiding this comment.
Refine TS typedef according to https://github.com/loopbackio/loopback-connector-openapi/blob/88cbea3ebcfe0fd7634e3fb49245bf8c15aa6a7a/README.md#extend-a-model-to-wrapmediate-api-operations
The striken out paragraph above is wrong - The linked resource is for loopback.remoteMethod, which is not related to DataSource.defineOperation.
There was a problem hiding this comment.
Refine scope according to
loopback-datasource-juggler/lib/datasource.js
Line 187 in be09d57
| accepts: string[], | |
| returns: string[], | |
| http: object, | |
| remoteEnabled: boolean, | |
| scope: unknown, | |
| fnName: string, | |
| accepts: string[], | |
| returns: string[], | |
| http: object, | |
| remoteEnabled: boolean, | |
| scope: DataAccessObject, | |
| fnName: string, |
Note that we'll need to write DataAccessObject itself, which is only used to be mixin-ed into DataSource and BaseModel.
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
| indexes?: { | ||
| [indexJugglerName: string]: IndexDefinition | ||
| }; |
There was a problem hiding this comment.
This is incorrect. Index definitions are done under the index object prop. of the PropertyDefinition.
There was a problem hiding this comment.
| indexes?: { | |
| [indexJugglerName: string]: IndexDefinition | |
| }; | |
| indexes?: string[]; |
| [name: string]: PropertyDefinition | ||
| } | ||
|
|
||
| export interface IndexDefinition { |
There was a problem hiding this comment.
As stated above, there is no separate "IndexDefinition". Instead, this is integrated under the "PropertyDefinition"
types/model.d.ts
Outdated
| | Function | ||
| | {[property: string]: PropertyType}; | ||
|
|
||
| export type DefaultFns = 'guid' | 'uuid' | 'uuidv4' | 'now' | 'shortid' | 'nanoid' | string; |
There was a problem hiding this comment.
Let's document which versions of Juggler onwards were these introduced.
| static definition: ModelDefinition; | ||
| static hideInternalProperties?: boolean; | ||
| static readonly base: typeof ModelBase; | ||
|
|
There was a problem hiding this comment.
Add missing protected attributes:
protected __cachedRelations: Record<string, unknown>;
protected __strict: boolean;
protected __persisted: boolean;
protected __unknownProperties: string[];
types/model.d.ts
Outdated
| export interface PropertyDefinition extends AnyObject { | ||
| type?: PropertyType; | ||
| id?: boolean | number; | ||
| defaultFn?: DefaultFns; |
There was a problem hiding this comment.
Add missing property
| defaultFn?: DefaultFns; | |
| /** | |
| * @remarks | |
| * | '$now' | Sets default to `new Date()`. Only applicable when {@link PropertyDefinition.type.name} equals `Date`. | |
| **/ | |
| default?: Function | Date | '$now'; | |
| defaultFn?: DefaultFns; |
see:
loopback-datasource-juggler/lib/model.js
Lines 281 to 300 in f8d7ca9
| applySetters?: boolean; | ||
| applyDefaultValues?: boolean; | ||
| strict?: boolean; | ||
| persisted?: boolean; |
There was a problem hiding this comment.
| applySetters?: boolean; | |
| applyDefaultValues?: boolean; | |
| strict?: boolean; | |
| persisted?: boolean; | |
| /** | |
| * @defaultValue `true` | |
| **/ | |
| applySetters?: boolean; | |
| /** | |
| * Configures if {@Link ModelBase.definition.settings.default} and {@Link ModelBase.definition.settings.defaultFn} should be used to populate yet-to-be-set model properties. | |
| * @defaultValue `true` | |
| **/ | |
| applyDefaultValues?: boolean; | |
| /** | |
| * @defaultValue {@Link ModelBase.definition.settings.strict} | |
| **/ | |
| strict?: boolean; | |
| persisted?: boolean; |
This is a mostly reverse-engineering effort to document the contracts in Juggler on a best-effort basis.
Signed-off-by: Rifa Achrinza 25147899+achrinza@users.noreply.github.com
Checklist
npm testpasses on your machine