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.

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

feat: add TS types #1904

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

feat: add TS types #1904

wants to merge 26 commits into from

Conversation

achrinza
Copy link
Member

@achrinza achrinza commented Sep 22, 2021

This is a mostly reverse-engineering effort to document the contracts in Juggler on a best-effort basis.

Signed-off-by: Rifa Achrinza [email protected]

Checklist

  • Sign off your commits with DCO (Developer Certificate of Origin)
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1269225996

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 84.747%

Totals Coverage Status
Change from base Build 1261591086: -0.007%
Covered Lines: 7159
Relevant Lines: 8149

💛 - Coveralls

@coveralls
Copy link

coveralls commented Sep 24, 2021

Pull Request Test Coverage Report for Build 1269236745

Warning: 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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 86.464%

Totals Coverage Status
Change from base Build 1261591086: 0.0%
Covered Lines: 7008
Relevant Lines: 7701

💛 - Coveralls

/**
* @internal
*/
_models?: Record<string, ModelBaseClass>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most probably ModelDefinition.

types/connector.d.ts Outdated Show resolved Hide resolved
@@ -76,28 +86,194 @@ import {IsolationLevel, Transaction} from './transaction-mixin';
*/
export declare class DataSource extends EventEmitter {
name: string;
settings: Options;
settings: ConnectorSettings;
Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the codebase, the DataSource simply passes the whole settings to the connector; Hence why the type called ConnectorSettings.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, we may want to create a dedicated passthrough type to avoid potential future issues due to the shared type.

Comment on lines 178 to 189
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Member Author

Choose a reason for hiding this comment

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

There's also a IdDefinition in model.d.ts. Not sure if this function is supposed to return that instead.

types/model.d.ts Outdated Show resolved Hide resolved
Comment on lines 248 to 249
freezeDataSource?(): void;
freezeSchema?(): void;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure the difference between these functions. DataSource.prototype.freeze calls both functions if they are defined.

Copy link
Member Author

@achrinza achrinza Sep 24, 2021

Choose a reason for hiding this comment

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

freezeDatasource seems to have been introduced by this commit:

9b169ef#diff-7de38f47065ef67b4936fbd6396bb71a580fe9a5a9ff2c9723c4fe859d1e2a6eR977-R979

freezeSchema was from JugglingDB.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

6af4b1b

Comment on lines 87 to 100
// #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>,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These are defined here:

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.

@achrinza achrinza changed the title feat: add connector TS types feat: add TS types Sep 24, 2021
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
types/connector.d.ts Outdated Show resolved Hide resolved

defineOperation(name: string, options: OperationOptions, fn: Function): void;

enableRemote(operation: string): void;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should deprecate as an LB3-only feature.


enableRemote(operation: string): void;

disableRemote(operation: string): void;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should deprecate as an LB3-only feature.

types/datasource.d.ts Show resolved Hide resolved
Comment on lines +527 to +528
* {@link Connector.discoverModelProperties} is used instead. Otherwise, an
* error is thrown.
Copy link
Member Author

Choose a reason for hiding this comment

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

#TODO: What error is thrown?

Comment on lines 153 to 155
/**
* A hash-map of the different relation types.
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's include an example since it's not expected to change.

types/datasource.d.ts Outdated Show resolved Hide resolved
types/datasource.d.ts Outdated Show resolved Hide resolved
Comment on lines 95 to 97
initialized?: boolean;
connected?: boolean;
connecting?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Add tsdoc explaining the circumstances for the possible values.

/**
* Base connector class
*
* @internal
*/
export declare class ConnectorBase implements Connector {
Copy link
Member Author

Choose a reason for hiding this comment

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

May need to extract this to loopback-connector package along with transaction-related typedefs.

Signed-off-by: Rifa Achrinza <[email protected]>
/**
* Opt-out unless stated otherwise
*/
export interface ConnectorCapabilities {
Copy link
Member Author

Choose a reason for hiding this comment

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

Separate between DAO and KVAO connector capabilities.

types/model.d.ts Show resolved Hide resolved
types/model.d.ts Outdated
excludeBaseProperties?: string[];

/**
* Indicates if the {@link ModelBaseClass | Model} is attached to the DataS
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix this tsdoc

* Alias of {@link ModelSettings.base}. Takes lower precedence.
*/
super?: ModelBaseClass;
excludeBaseProperties?: string[];
Copy link
Member Author

Choose a reason for hiding this comment

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

Add tsdoc

* Model properties to be set as hidden.
*
* @remarks
* Hidden properties are
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix incomplete tsdoc

types/model.d.ts Outdated Show resolved Hide resolved
types/model.d.ts Outdated
Comment on lines 107 to 111
plural?: string;

http?: {
path?: string;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Comment on lines 122 to 124
// Postgresql-specific
type: string;
kind: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, add tsdoc

types/model.d.ts Outdated Show resolved Hide resolved
Comment on lines +19 to +21
maxDepthOfQuery?: number;
maxDepthOfData?: number;
prohibitHiddenPropertiesInQuery?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Explain these options with tsdoc.

Comment on lines +155 to +168
// #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>;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines 18 to 25
export type OperationOptions = {
accepts: string[],
returns: string[],
http: object,
remoteEnabled: boolean,
scope: unknown,
fnName: string,
}
Copy link
Member Author

@achrinza achrinza Apr 2, 2022

Choose a reason for hiding this comment

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

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.

types/datasource.d.ts Outdated Show resolved Hide resolved
Comment on lines 529 to 533
buildModelFromInstance(
modelName: string,
jsonObject: AnyObject,
options?: Options,
): ModelBaseClass;
Copy link
Member Author

Choose a reason for hiding this comment

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

Add documentation on what (Connector?) methods this calls behind the scenes.

@@ -76,28 +86,194 @@ import {IsolationLevel, Transaction} from './transaction-mixin';
*/
export declare class DataSource extends EventEmitter {
name: string;
settings: Options;
settings: ConnectorSettings;
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, we may want to create a dedicated passthrough type to avoid potential future issues due to the shared type.

types/datasource.d.ts Show resolved Hide resolved
types/datasource.d.ts Outdated Show resolved Hide resolved
types/connector.d.ts Outdated Show resolved Hide resolved
* @deprecated Use {@link ConnectorSettings.connector} instead.
*/
adapter?: ConnectorStatic | string;
database: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Add tsdoc explaining that this is used by the Connectors directly.

types/connector.d.ts Show resolved Hide resolved
types/connector.d.ts Show resolved Hide resolved
types/connector.d.ts Show resolved Hide resolved
@@ -219,7 +405,11 @@ export declare class ModelBase {
* If true, then protected properties should not be brought out.
* @returns {object} returns Plain JSON object
Copy link
Member Author

Choose a reason for hiding this comment

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

Convert to tsdoc

types/model.d.ts Outdated
* Comma-separated column names
*
* @remarks
* Handled by {@link Connector}s directly by default.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.
Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto. What does "by default" mean? What is the circumstance where this is not the case, and what is the impact?

/**
* @internal
*/
_models?: Record<string, ModelBaseClass>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Most probably ModelDefinition.

Comment on lines 112 to 114
models: Record<string, typeof ModelBase>;

definitions: {[modelName: string]: ModelDefinition};
Copy link
Member Author

Choose a reason for hiding this comment

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

Will need to check and document these' relationship with Connector._models

Copy link
Member Author

Choose a reason for hiding this comment

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

These are populated from the same-named properties of the ModelBuilder instance passed during class initialisation.

types/connector.d.ts Show resolved Hide resolved
types/model-utils.d.ts Show resolved Hide resolved
types/connector.d.ts Show resolved Hide resolved
types/model.d.ts Outdated Show resolved Hide resolved
achrinza added 2 commits April 9, 2022 22:36
Signed-off-by: Rifa Achrinza <[email protected]>
@achrinza
Copy link
Member Author

achrinza commented May 9, 2022

JugglingDB documentation: https://1602.github.io/jugglingdb/#DOCUMENTATION

* @remarks
* Alias of {@link ModelSettings.base}. Takes lower precedence.
*/
super?: ModelBaseClass;
Copy link
Member Author

Choose a reason for hiding this comment

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

todo: Document that this convention is from node:util.inherits.

* @param options Discovery options
* @param cb Callback function
*/
discoverSchemas?(tableName: string, options: SchemaDiscoveryOptions, cb: Callback<Schema>): Promise<Schema>;
Copy link
Member Author

Choose a reason for hiding this comment

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

clarification: Are the 2nd and 3rd parameters supposed to be optional? Check the return type as well.

Suggested change
discoverSchemas?(tableName: string, options: SchemaDiscoveryOptions, cb: Callback<Schema>): Promise<Schema>;
discoverSchemas?(tableName: string, options?: SchemaDiscoveryOptions, cb?: Callback<Schema>): Promise<Schema>;

Comment on lines 177 to 185
constructor(name: string, settings?: ConnectorSettings, modelBuilder?: ModelBuilder);

constructor(settings?: ConnectorSettings, modelBuilder?: ModelBuilder);

constructor(
connectorModule: Connector,
settings?: Options,
settings?: Omit<ConnectorSettings, 'adapter' | 'connector'>,
modelBuilder?: ModelBuilder,
);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

types/model.d.ts Outdated
Comment on lines 182 to 186
/**
* Sets if JavaScript {@link undefined} as an attribute value should be
* persisted as database `NULL`.
*/
persistUndefinedAsNull?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Member Author

@achrinza achrinza May 20, 2022

Choose a reason for hiding this comment

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

What's the default value? IIRC, it's true as LoopBack 4's DefaultCrudRepository explicitly sets this to false:

https://github.com/loopbackio/loopback-next/blob/1e48456fde20e691d85b506f5c8e296c3c64b1fe/packages/repository/src/repositories/legacy-juggler-bridge.ts#L217

@@ -7,21 +7,37 @@ import {EventEmitter} from 'events';
import {AnyObject, Options} from './common';
import {DataSource} from './datasource';
import {Listener, OperationHookContext} from './observer-mixin';
import {ModelUtilsOptions} from './model-utils';

Copy link
Member Author

Choose a reason for hiding this comment

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

patch: boolean;
}; // Only referenced in "legacy built-in merge policy"
options?: {
path: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a typo

Suggested change
path: boolean;
patch: boolean;

types/model.d.ts Outdated
Comment on lines 172 to 180
/**
* {@inheritDoc ModelSettings.tableName}
*/
tableName?: string

/**
* Mapped table name for the model.
*/
table?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Which one takes precedence?

Comment on lines 20 to 25
accepts: string[],
returns: string[],
http: object,
remoteEnabled: boolean,
scope: unknown,
fnName: string,
Copy link
Member Author

@achrinza achrinza May 21, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refine scope according to

scope: this.DataAccessObject.prototype,
.

Suggested change
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 <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
Signed-off-by: Rifa Achrinza <[email protected]>
types/model.d.ts Outdated
Comment on lines 157 to 159
indexes?: {
[indexJugglerName: string]: IndexDefinition
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is incorrect. Index definitions are done under the index object prop. of the PropertyDefinition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
indexes?: {
[indexJugglerName: string]: IndexDefinition
};
indexes?: string[];

@@ -66,6 +82,31 @@ export interface ModelProperties {
[name: string]: PropertyDefinition
}

export interface IndexDefinition {
Copy link
Member Author

Choose a reason for hiding this comment

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

As stated above, there is no separate "IndexDefinition". Instead, this is integrated under the "PropertyDefinition"

types/model.d.ts Outdated
| string
| Function
| {[property: string]: PropertyType};

export type DefaultFns = 'guid' | 'uuid' | 'uuidv4' | 'now' | 'shortid' | 'nanoid' | string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's document which versions of Juggler onwards were these introduced.

/**
* Base class for LoopBack 3.x models
*/
export declare class ModelBase {
static dataSource?: DataSource;
static modelName: string;
static definition: ModelDefinition;
static hideInternalProperties?: boolean;
static readonly base: typeof ModelBase;

Copy link
Member Author

Choose a reason for hiding this comment

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

Add missing protected attributes:

protected __cachedRelations: Record<string, unknown>;
protected __strict: boolean;
protected __persisted: boolean;
protected __unknownProperties: string[];

types/model.d.ts Outdated
/**
* Property definition
*/
export interface PropertyDefinition extends AnyObject {
type?: PropertyType;
id?: boolean | number;
defaultFn?: DefaultFns;
Copy link
Member Author

Choose a reason for hiding this comment

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

Add missing property

Suggested change
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:

if (applyDefaultValues && propVal === undefined && appliesDefaultsOnWrites(properties[p])) {
let def = properties[p]['default'];
if (def !== undefined) {
if (typeof def === 'function') {
if (def === Date) {
// FIXME: We should coerce the value in general
// This is a work around to {default: Date}
// Date() will return a string instead of Date
def = new Date();
} else {
def = def();
}
} else if (type.name === 'Date' && def === '$now') {
def = new Date();
}
// FIXME: We should coerce the value
// will implement it after we refactor the PropertyDefinition
self.__data[p] = propVal = def;
}
}

Comment on lines +286 to +289
applySetters?: boolean;
applyDefaultValues?: boolean;
strict?: boolean;
persisted?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants