Add TypeScript declaration files #1578

raymondfeng · 2018-05-03T14:54:35Z

Description

This PR adds basic TypeScript declarations for juggler related types to be referenced in LoopBack 4.

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

zbarbuto

LGTM - would be awesome to get typedefs out of the box. Especially for loopback core as well for things like User, Role and app.

I did notice Model is missing a few of the static methods like remoteMethod, nestRemoting and getApp.

zbarbuto · 2018-05-03T23:17:19Z

types/scope.d.ts

+export interface ScopedMethods<T extends ModelBase = ModelBase> {
+  build(data: ModelData<T>): T;
+
+  create(


What about the overloads for all of these methods? E.g. you can technically skip options and just provide the callback:

create(data: ModelData<T>, callback: Callback<T>): PromiseOrVoid<T>;

my preference would be to explicitly list these so that if you don't pass in a callback you know you're getting a promise:

create(data: ModelData<T>, options?: Options): Promise<T>; create(data: ModelData<T>, callback: Callback<T>): void; create(data: ModelData<T>, options: Options, callback: Callback<T>): void;

Also, I believe create also supports arrays of data for bulk creates:

create(data: ModelData<T> | ModelData<T>[]): Promise<T>;

@zbarbuto Thank you for chiming in. It's a good idea to use overloaded method signatures. Let's do it incrementally. I would like to use the type information for LoopBack 4 soon.

For loopback module, I think there is already a community module - @types/loopback. Do you want to check out?

Yes have used this in the past - it does have some issues. Was only mentioning in case it was on the cards to eventually have typedefs out of the box in loopback as well.

Understood re: changing incrementally.

@zbarbuto I don't think I include Model in the declaration files. Please note that ModelBase is different from Model:

ModelBase: a base class in loopback-datasource-juggler as the common root of all models

Model: a class in loopback

bajtos

Great start!

I did not review the actual types in details, it's too difficult to verify their correctness manually. Can we mark TypeScript support as experimental to allow us to make backwards-incompatible fixes in semver-minor versions?

I have few high-level comments and topics to discuss, see below.

bajtos · 2018-05-04T11:35:33Z

.prettierrc

@@ -0,0 +1,6 @@
+{
+  "bracketSpacing": false,
+  "singleQuote": true,


Let's not mix adding Prettier with typings. Could you please open a new PR adding Prettier to our workflow (including npm test executed by CI)? Alternatively, keep these prettier-related files local only to help your IDE, don't commit them to git.

bajtos · 2018-05-04T11:36:48Z

tsconfig.json

@@ -0,0 +1,18 @@
+{
+  "$schema": "http://json.schemastore.org/tsconfig",
+  "compilerOptions": {


I am confused why do we need a tsconfig file, when this project does not contain any TypeScript sources to compile, only .d.ts files. Could you please clarify?

The presence of tsconfig.json file allows VsCode to validate the syntax of .d.ts files.

Cool!

I think we should have a build step to run that validations, otherwise our .d.ts files can quickly become invalid. I consider this as a must have for this pull request.

Ah, I see you have added npm run tsc, we are good here 👍

bajtos · 2018-05-04T11:38:27Z

types/common.d.ts

+ *
+ * Note that juggler uses Bluebird, not the native Promise.
+ */
+export type PromiseOrVoid<T = any> = PromiseLike<T> | void;


I don't see any import or definition of PromiseLike<T>, perhaps an oversight on your side?

I think we need to introduce some kind of a smoke test (as part of this pull request!) to at least verify that type definitions can be parsed & loaded by tsc.

PromiseLike is a built-in type from lib.es5.d.ts.

I added npm run tsc to validate *.d.ts files.

bajtos · 2018-05-04T11:39:26Z

types/datasource.d.ts

+ * 3. new DataSource(connectorModule, settings). For example:
+ *   - new DataSource(connectorModule, {name: 'myDataSource})
+ *   - new DataSource(connectorModule)
+ */


Is it a good idea to duplicate the jsdoc comments in typing files? I am concerned that the content in .d.ts files will quickly get out of sync with the original comments in .js files.

When the package is used by other modules such as @loopback/repository, VSCode uses the tsdocs from .d.ts file, IIUC.

I see, that makes sense.

It would be great to find a way how to automatically sync these jsdoc/tsdoc comments, but that's out of scope of this pull request.

bajtos · 2018-05-04T11:40:59Z

types/model.d.ts

+import {EventEmitter} from 'events';
+
+// tslint:disable:no-any
+// tslint:disable:max-line-length


AFAICT, we don't have tslint integration in juggler, therefore we should not need any tslint overrides.

bajtos · 2018-05-04T11:46:03Z

types/persisted-model.d.ts

+  count: number;
+}
+
+export declare class PersistedModel extends ModelBase {


Technically speaking, there is no such thing as a PersistedModel at juggler level. We have ModelBase and DataAccessObject mixin that's added to ModelBase when a model is attached to a datasource using the default DAO implementation. Similarly, we have KeyValueAccessObject mixin that provides API for models connected to key-value stores.

The class PersistedModel is implemented in loopback, as a thin wrapper providing default DAO methods which throw a helpful message when the model was not attached to a datasource yet.

In the light of that, I feel the currently proposed typedefs are subtly incorrect and I would prefer if we could find a way how to make .d.ts files closer to the actual juggler API and behavior.

Thoughts?

LB3 uses mixins at runtime to build model classes. Even though the PersistedModel is physically from loopback module, we do have the assumption of PersistedModel class in many cases in loopback-datasource-juggler.

It's hard to describe such contracts in other forms such as interfaces to allow both static and prototype methods/properties.

bajtos · 2018-05-09T14:06:31Z

LB3 uses mixins at runtime to build model classes. Even though the PersistedModel is physically from loopback module, we do have the assumption of PersistedModel class in many cases in loopback-datasource-juggler.

It's hard to describe such contracts in other forms such as interfaces to allow both static and prototype methods/properties.

I'll see. This is an interesting problem, I may give it a try myself. For now, I am ok with a suboptimal solution that works right now.

bajtos · 2018-05-09T14:07:36Z

.gitignore

@@ -13,5 +13,5 @@ npm-debug.log
 .project
 test/memory.json
 .nyc_output
-
+dist


Please add dist to .npmignore too.

bajtos · 2018-05-09T14:29:48Z

tsconfig.json

@@ -0,0 +1,18 @@
+{
+  "$schema": "http://json.schemastore.org/tsconfig",
+  "compilerOptions": {


Ah, I see you have added npm run tsc, we are good here 👍

bajtos · 2018-05-09T14:33:22Z

Few more comments (see above), I think this is almost ready to be landed. My expectation is that we will be fixing any bugs as we discover them from real-world usage, thus this initial typings can be landed quickly.

I feel it's really important to clearly set the expectations about the stability of the TypeScript type definitions. Could you please add a bold comment to index.d.ts to mark TypeScript support as experimental and explicitly tell the users that backwards-incompatible fixes may come in semver-minor versions?

b-admike

Awesome stuff @raymondfeng. I just looked at datasource.d.ts and was wondering why we have some missing methods compared to https://apidocs.strongloop.com/loopback-datasource-juggler/#datasource? Perhaps it's because we'd like to add them incrementally?

b-admike · 2018-05-09T16:11:14Z

types/datasource.d.ts

+  connected?: boolean;
+  connecting?: boolean;
+
+  connector?: any;


Can we bring in https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/connectors/connector.ts for this?

raymondfeng · 2018-05-09T19:48:47Z

@bajtos @zbarbuto @b-admike Any more comments before I merge the PR?

b-admike

LGTM 👍 , although I haven't looked through all the details. I'm good with the plan to modify incrementally.

zbarbuto · 2018-05-09T23:13:55Z

Agree with @b-admike - LGTM for a first-pass

bajtos

raymondfeng requested review from b-admike, dhmlau, jannyHou, shimks, virkt25 and zbarbuto as code owners May 3, 2018 14:54

raymondfeng changed the title ~~Add TypeScript declaration files~~ [WIP] Add TypeScript declaration files May 3, 2018

raymondfeng mentioned this pull request May 3, 2018

feat: add integration for service-oriented backends loopbackio/loopback-next#1119

Merged

6 tasks

Tidy up datasource

cc99cd8

raymondfeng force-pushed the ts-definitions branch from 8a43ce1 to 1fe5308 Compare May 3, 2018 21:15

zbarbuto reviewed May 3, 2018

View reviewed changes

bajtos suggested changes May 4, 2018

View reviewed changes

raymondfeng force-pushed the ts-definitions branch 3 times, most recently from a11a3fb to ab3412d Compare May 8, 2018 21:39

bajtos reviewed May 9, 2018

View reviewed changes

b-admike reviewed May 9, 2018

View reviewed changes

raymondfeng force-pushed the ts-definitions branch 2 times, most recently from d0f08c4 to ebcf6de Compare May 9, 2018 18:14

Add typescript type definitions

77a2c9a

raymondfeng force-pushed the ts-definitions branch from ebcf6de to 77a2c9a Compare May 9, 2018 19:47

b-admike approved these changes May 9, 2018

View reviewed changes

zbarbuto approved these changes May 9, 2018

View reviewed changes

bajtos approved these changes May 10, 2018

View reviewed changes

raymondfeng changed the title ~~[WIP] Add TypeScript declaration files~~ Add TypeScript declaration files May 10, 2018

raymondfeng merged commit c955802 into master May 10, 2018

raymondfeng deleted the ts-definitions branch May 10, 2018 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript declaration files #1578

Add TypeScript declaration files #1578

raymondfeng commented May 3, 2018 •

edited

Loading

zbarbuto left a comment •

edited

Loading

zbarbuto May 3, 2018

raymondfeng May 4, 2018

zbarbuto May 4, 2018

raymondfeng May 8, 2018

bajtos left a comment

bajtos May 4, 2018

bajtos May 4, 2018

raymondfeng May 8, 2018

bajtos May 9, 2018

bajtos May 9, 2018

bajtos May 4, 2018

raymondfeng May 8, 2018

raymondfeng May 8, 2018

bajtos May 9, 2018

bajtos May 4, 2018

raymondfeng May 8, 2018

bajtos May 9, 2018

bajtos May 4, 2018

bajtos May 4, 2018

raymondfeng May 8, 2018

raymondfeng May 8, 2018

bajtos commented May 9, 2018

bajtos May 9, 2018

bajtos May 9, 2018

bajtos commented May 9, 2018

b-admike left a comment

b-admike May 9, 2018

raymondfeng May 9, 2018

raymondfeng commented May 9, 2018

b-admike left a comment

zbarbuto commented May 9, 2018

bajtos left a comment

Add TypeScript declaration files #1578

Add TypeScript declaration files #1578

Conversation

raymondfeng commented May 3, 2018 • edited Loading

Description

Related issues

Checklist

zbarbuto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented May 9, 2018

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented May 9, 2018

b-admike left a comment

Choose a reason for hiding this comment

zbarbuto commented May 9, 2018

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented May 3, 2018 •

edited

Loading

zbarbuto left a comment •

edited

Loading