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

Add TypeScript definitions with flow-to-ts #7337

Closed
wants to merge 3 commits into from

Conversation

sadortun
Copy link
Contributor

New Pull Request Checklist

Issue Description

Tests with flow-to-ts.

You can test out by running

npm run flow-to-type && npm run types

This run well, except npx typescript ...

Found 1426 errors.

Related issue: #7334 #7335 #7336

Approach

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • ...

@sadortun sadortun changed the title Tests with flow-to-ts Generate Typescript types definitions using flow-to-ts && tsc Apr 11, 2021
@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #7337 (874f553) into master (c66a39f) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7337      +/-   ##
==========================================
- Coverage   93.97%   93.94%   -0.04%     
==========================================
  Files         181      181              
  Lines       13271    13271              
==========================================
- Hits        12471    12467       -4     
- Misses        800      804       +4     
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.37% <0.00%> (-0.66%) ⬇️
src/RestWrite.js 93.92% <0.00%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c66a39f...874f553. Read the comment docs.

@sadortun
Copy link
Contributor Author

I had a look at the errors, and what it shows is even if parse-server uses flow, types declarations are only used sparsely.

The most suitable way forward, would probably be to complete type definitions in current flow codebase and once we can reduce this number of errors to almost zero, use this to generate types, and/or convert the project to Typescript.

I dont think there is too much drawbacks in adding types definitions to existing code. But this is a long-term effort toward Typescript

@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

Are you saying that using flow-to-ts mixes point 2 with 3 or even require 3 before 2?

That is what I meant and what makes this approach so difficult. If this is really the case, then we should probably use #7335 which solves this by decoupling flow from TS. This can be done in maybe a couple of hours and I think the time is better spent than in #7336.

Now the other approach may be to first do 3 and solve 2 afterwards, but 3 is a completely different challenge.

Can you post some of these errors to see what they are?

@sadortun
Copy link
Contributor Author

Ok perfect! #7335 FTW !

@sadortun sadortun closed this Apr 11, 2021
@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

The most suitable way forward, would probably be to complete type definitions in current flow codebase and once we can reduce this number of errors to almost zero, use this to generate types, and/or convert the project to Typescript.

You mean the errors occur because some parts of the code were not flow type annotated?

@sadortun
Copy link
Contributor Author

Yes

@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

The flow-to-ts module has no option to simply ignore non-annotated code?

@sadortun
Copy link
Contributor Author

flow-to-ts convert current JS code to TS. If there is no annotations in the original JS, there is also no annotations in target TS.

When you run TSC after that, it generate errors because of missing annotations, or use of any types.

You can either add types now in flow, and re-run flow-to-ts then TSC, or just add annotations in TS and re-run TSC.

In both cases, you need to add annotations (in current JS, or TS) to end up with valid typescript code.

From what I understand, Parse want to move toward Typescript at some point in time, of you want to, you can start to progressively, in small steps without breaking changes, add types in current JS, and test by running flow-to-ts then TSC until you have no more errors.

If you don't care much about Typescript, you can generate definitions from JSDoc. But dooing so will not make your codebase Typescript, it will only allow users to have definitions to use in theirs projects.

@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

When you run TSC after that, it generate errors because of missing annotations, or use of any types.

Oh, the errors do not come from flow-to-ts but from TS? That's surprising because in #7335 there are no annotations at all and TS does not throw errors. Maybe a bug in flow-to-type as it's still WIP? For any we could just strip them maybe.

@sadortun
Copy link
Contributor Author

That's surprising because in #7335 there are no annotations at all and TS does not throw errors.

  • tsc parsing JS/JSDoc will only do its "best" to output something, it wont raise errors when parsing code, as you can see bellow, about everything is `any.

  • tsc parsing Typescript .ts will assert that everything is type-safe. The 1442 errors (see bellow) generated are because of type misc-match in parse-server code base.

Quick comparaison

Code generated from your #7335 branch using JSDoc

You can see that about everything is any. There is probably some config to tweak, but its probably a good 100+ hours to get the generated definitions to be usable.

/// <reference types="node" />
export var __esModule: boolean;
export default _default;
declare var _default: typeof ParseServer;
declare class ParseServer {
    /**
     * @static
     * Create an express app for the parse server
     * @param {Object} options let you specify the maxUploadSize when creating the express app  */
    static app(options: any): import("express-serve-static-core").Express;
    static promiseRouter({ appId }: {
        appId: any;
    }): any;
    /**
     * Creates a new ParseServer and starts it.
     * @param {ParseServerOptions} options used to start the server
     * @param {Function} callback called when the server has started
     * @returns {ParseServer} the parse server instance
     */
    static start(options: any, callback: Function): ParseServer;
    /**
     * Helper method to create a liveQuery server
     * @static
     * @param {Server} httpServer an optional http server to pass
     * @param {LiveQueryServerOptions} config options for the liveQueryServer
     * @param {ParseServerOptions} options options for the ParseServer
     * @returns {ParseLiveQueryServer} the live query server instance
     */
    static createLiveQueryServer(httpServer: any, config: any, options: any): any;
    static verifyServerUrl(callback: any): void;
    /**
     * @constructor
     * @param {ParseServerOptions} options the parse server initialization options
     */
    constructor(options: any);
    config: any;
    get app(): import("express-serve-static-core").Express;
    _app: import("express-serve-static-core").Express;
    handleShutdown(): Promise<void>;
    /**
     * starts the parse server's express app
     * @param {ParseServerOptions} options to use to start the server
     * @param {Function} callback called when the server has started
     * @returns {ParseServer} the parse server instance
     */
    start(options: any, callback: Function): ParseServer;
    server: import("http").Server;
    liveQueryServer: any;
    expressApp: import("express-serve-static-core").Express;
}

Using this #7337 and flow-to-js

Not perfect, but closer of what we should aim with Typescript. And this do gene

import { ParseServerOptions, LiveQueryServerOptions } from "./Options";
import PromiseRouter from "./PromiseRouter";
import { ParseLiveQueryServer } from "./LiveQuery/ParseLiveQueryServer";

export interface ParseServerOptions {
  /* Your Parse Application ID
  :ENV: PARSE_SERVER_APPLICATION_ID */
  appId: string;

  /* Your Parse Master Key */
  masterKey: string;

  /* URL to your parse server with http:// or https://.
  :ENV: PARSE_SERVER_URL */
  serverURL: string;

  /* Restrict masterKey to be used by only these ips, defaults to [] (allow all ips)
  :DEFAULT: [] */
  masterKeyIps: string[] | null | undefined;

  /* Sets the app name */
  appName: string | null | undefined;

  /* Add headers to Access-Control-Allow-Headers */
  allowHeaders: string[] | null | undefined;

/// .... 500 lines ....
}

declare class ParseServer {
    /**
     * @constructor
     * @param {ParseServerOptions} options the parse server initialization options
     */
    constructor(options: ParseServerOptions);
    get app(): any;
    handleShutdown(): Promise<void>;
    /**
     * @static
     * Create an express app for the parse server
     * @param {Object} options let you specify the maxUploadSize when creating the express app  */
    static app(options: any): any;
    static promiseRouter({ appId }: {
        appId: any;
    }): PromiseRouter;
    /**
     * starts the parse server's express app
     * @param {ParseServerOptions} options to use to start the server
     * @param {Function} callback called when the server has started
     * @returns {ParseServer} the parse server instance
     */
    start(options: ParseServerOptions, callback: (() => void) | null | undefined): this;
    /**
     * Creates a new ParseServer and starts it.
     * @param {ParseServerOptions} options used to start the server
     * @param {Function} callback called when the server has started
     * @returns {ParseServer} the parse server instance
     */
    static start(options: ParseServerOptions, callback: (() => void) | null | undefined): ParseServer;
    /**
     * Helper method to create a liveQuery server
     * @static
     * @param {Server} httpServer an optional http server to pass
     * @param {LiveQueryServerOptions} config options for the liveQueryServer
     * @param {ParseServerOptions} options options for the ParseServer
     * @returns {ParseLiveQueryServer} the live query server instance
     */
    static createLiveQueryServer(httpServer: any, config: LiveQueryServerOptions, options: ParseServerOptions): ParseLiveQueryServer;
    static verifyServerUrl(callback: any): void;
}
export default ParseServer;

Examples of errors :

tmp-ts/src/Controllers/DatabaseController.ts:1472:16 - error TS2339: Property 'then' does not exist on type 'true | Promise<void>'.
  Property 'then' does not exist on type 'true'.

1472               .then(() =>

tmp-ts/src/Controllers/FilesController.ts:6:8 - error TS1192: Module '"E:/prog/projets/gpl/parse-server/node_modules/@types/mime/index"' has no default export.

6 import mime from "mime";


         ~~~~
tmp-ts/src/Controllers/index.ts:112:34 - error TS2554: Expected 3 arguments, but got 2.

112   const filesControllerAdapter = loadAdapter(filesAdapter, () => {
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
113     return new GridFSBucketAdapter(databaseURI, {}, fileKey);
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
114   });
    ~~~~


@sadortun
Copy link
Contributor Author

sadortun commented Apr 11, 2021

Quick follow up,

If the goal for now is only to generate Typescript definitions, and NOT replace the codebase with Typescript entierly (aka case (2)) :

We can use the definitions as they are generated currently and ignore TSC 1440 errors.

That way, we can improve annotations in current flow/JS code and regenerate the type definitions.

I think I was mistaken when I mentioned that we need to do (3) before (2). After looking more in details, it seems that despite errors, the types definitions and interfaces are generated correctly.

By focusing the efforts on public-facing APIs (ParseServer, Cloud code ....) , I think it can be manageable.


Later on, we can fix those 1440 TSC errors and maybe by parse-server v6.0.0, we could transition to Typescript entirely.

@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

If the goal for now is only to generate Typescript definitions, and NOT replace the codebase with Typescript entierly (aka case (2)) :

That is what I believe. Replacing the code base with TS would be (3).

The goal of #7335 is not (3) but (2) by simply making sure that public properties have a proper JSDoc and that also generates the TS files. We need JSDoc anyway for docs and IDE, and as part of (3) we can convert JSDoc to TSDoc. So adding JSDoc would also be beneficial for docs quality.

#7337 of course has more types because it is using the flow types (which are stripped and would need to be added as JSDoc in #7335), but the question for me is how can we move from #7337 to (3)?

  • From Add TypeScript definitions with JSDoc #7335 it should be easy because of the decoupling. And any JSDoc we add will help to make the docs more complete.
  • From Add TypeScript definitions with flow-to-ts #7337 we have flow types -> flow-to-ts -> ts types. Now if we want to add more types, would we have to add flow types or can we already add TS types and have a mix? Because any flow type that we add from now on will have to be changed to TS type again, that's double work? Or do we keep adding flow types and when we want to start with (3) we replace all source files with the output of flow-to-ts and remove flow-to-ts?

@sadortun
Copy link
Contributor Author

sadortun commented Apr 11, 2021

The goal of #7335 is not (3) but (2) by simply making sure that public properties have a proper JSDoc and that also generates the TS files

Correct me if in wrong, but from what I've seen, #7335 does generate JSDoc and .d.ts declarations, but thoses declarations even if they exists and contains parameters, everything is any which is in the Typescript world, fairly useless.

#7337 on the other hand can generate "good" quality .d.ts, and also JSDoc at the same time.

Its also possible to move toward (3) later next year.

The real magic behind .d.ts is that If you use the code wrong, it will raise compilation errors. Same for example if in some later version you add a new mandatory field to an object, the compilator will literally yell at you for that. 😁 versus JSDoc that might show a lint error if you're lucky.


And any JSDoc we add will help to make the docs more complete.

There is nothing in #7337 that will make JSDoc worst :) if you update them at the same time as you add types, you'll end up with awesome .d.ts and JSDoc too !


any flow type that we add from now on will have to be changed to TS type again, that's double work?

No, that's why flow-to-ts exists. You just keep adding flow types gradually over the next year, and re-run it to update definitions.

Unless you do really Wierd things in flow, flow-to-ts can manage the conversion to .ts /

So if you update the current flow code and JSDoc, nothing is lost and no extra* efforts are needed.

(*) I haven't do through ALL of flow-to-ts generated code, but from what I saw so far, it seems pretty good. If wou decide to go with #7337, it would be a good idea for one of the core team to do a review of flow-to-ts output. ( #7337 also include a AST parser so you can use that to tweak the conversion process, if needed. )


when we want to start with (3) we replace all source files with the output of flow-to-ts and remove flow-to-ts?

Exactly.

For example, If you start with (3) right now, you have 1440 errors to fix. If you start with #7337 / (2) instead, you can add types gradually , and at some point the 1440 errors will go down to zero.

@sadortun sadortun reopened this Apr 11, 2021
@ghost
Copy link

ghost commented Apr 11, 2021

Warnings
⚠️ Please add a changelog entry for your changes.

Generated by 🚫 dangerJS

@mtrezza
Copy link
Member

mtrezza commented Apr 11, 2021

Correct me if in wrong, but from what I've seen, #7335 does generate JSDoc and .d.ts declarations, but thoses declarations even if they exists and contains parameters, everything is any which is in the Typescript world, fairly useless.

It uses the existing types in JSDoc to generate TS. For example in Utils.d.ts the types are set.

#7337 on the other hand can generate "good" quality .d.ts, and also JSDoc at the same time.

Generate JSDoc? Or do you mean it just passes the JSDoc from the .js files through to the .d.ts files?

For example, If you start with (3) right now, you have 1440 errors to fix. If you start with #7337 / (2) instead, you can add types gradually , and at some point the 1440 errors will go down to zero.

Sounds good, so the process would be:

  1. Add missing flow annotations to achieve at least the same output as we currently have in DefinitelyTyped
    • That would not address all 1440 errors, but likely already reduce the number
    • We can ignore errors because despite them, everything is at least generated "as is" (albeit with type mismatch)
    • That means we reach status quo (no loss in TS definitions for developers)
    • Can we already ship Parse repos with the types set in package.json - what happens if a developer additionally includes the @types/... package?
  2. Add missing flow annotations to document all publicly exposed APIs
    • That is an improvement of status quo
    • We will still have many of the 1440 errors
  3. Fix all remaining TS errors
  4. Enable TS in the repo (3)
    • Replace all source files with the output of flow-to-ts
    • Does flow-to-ts also reformat? If so, thanks to eslint and prettier, we should be able to mitigate that and not make every line of the code base change.
    • Maybe we can already try to replace the source files with an output of flow-to-ts and see what happens; the module is a WIP, so some prospective dry-runs can help to verify our plans.
  5. Add CI check to fail on TS error
  6. Gradually complete TS annotations whenever we touch code for changes, unless somewhat makes a dedicated "Add missing TS annotations" PR

Something like that?

@sadortun
Copy link
Contributor Author

Generate JSDoc? Or do you mean it just passes the JSDoc from the .js files through to the .d.ts files?

Hehehe, yes :)

We can ignore errors because despite them, everything is at least generated "as is" (albeit with type mismatch)

This I my understanding. That said, adding types to interfaces/ methods will also "force" us to define them and then make it easier to rework/update the method internal types. (Aka, you set input params and return types, later you fix types compatibilities inside the methods. )

Once this is "done", the errors count should go down by a lot! And you almost have a TypeScript ready project.

Can we already ship Parse repos with the types set in package.json - what happens if a developer additionally includes the @types/... package?

I haven't figured that part yet, but I'm sure someone more knowledgeable than me in NPM can figure this out.

I'm from PHP, and PHP use Composer for package management . Composer have a replace and conflict

Map of packages that conflict with this version of this package. They will not be allowed to be installed together with your package.

Thats ensure the user uninstall @types/parse when updating.

I'm sure there is a way to do that here.

If not, I think the user may have an error saying the types are already defined.

Does flow-to-ts also reformat?

Mmm, not sure, you can try to do a diff need a if you want to test. In any cases, i guess you just run eslint after flow-to-ts ( you should do that anyway :) )

Something like that?

🚀🚀🚀 Yeahh 🚀🚀🚀 !!

@mtrezza
Copy link
Member

mtrezza commented Apr 12, 2021

Mmm, not sure, you can try to do a diff need a if you want to test. In any cases, i guess you just run eslint after flow-to-ts ( you should do that anyway :) )

Even after eslint and prettier there is usually some degree of formatting freedom. I think we should do a diff to understand what we'd replace the source files with.

I like this workflow, sounds better than #7335!

@sadortun
Copy link
Contributor Author

sadortun commented Apr 12, 2021

Even after eslint and prettier there is usually some degree of formatting freedom.

Indeed, I am pretty sure this diff would be quite a big chunk. And I would not have my hopes up to have a smooth diff. If you use JetBrains**, it have a really powerful formating tool. I would suggest using this to :

  1. Way in advance, like a few months before, reformat code progressively using a formating tool like JetBrains to iron out any variance. Jetbrain code formating can be tweaked to very fine details to match the codebase as close as possible.

  2. Transition to TS.

  3. Re-run Code formating.

This would reduce to a maximum the diff between JS and TS. Also, it worth noting that from what is currently used, flow types is really close to TS.

(**) Mentioning Jetbrain here, but it could be anything that have a really good formating tool. If you're not familiar, it have about 500 options for code formating.

Pro-Tip: you can get a OpenSource software lisence for all their products for free. ( If you use it only on OpenSource projects )


I like this workflow

So the good thing with this way to do things is that you can decide when you do the transition from .js to .ts

This also allow to put a "deadline for any PR to be merged before the transition.

In the case a PR is still pending, you can :

  1. In the main repo, git tag pre-transition-to-ts

  2. Do the transition to TS.

  3. Ask PR to rebase their PR on pre-transition-to-ts

  4. Run flow-to-ts on their PR.

  5. Review, test and Merge the PR.

@mtrezza
Copy link
Member

mtrezza commented Apr 12, 2021

JetBrains

Maybe something to consider, prettier is pretty opinionated.

This also allow to put a "deadline for any PR to be merged before the transition.

We could make sure that any PRs that extensively touch the code are merged before the transition to mitigate extra effort with rebases. After the transition to TS we should add a CI check, so PRs that reduce TS coverage or however we measure it, would not be merged without proper rebase.

Sounds good, so I'll close #7335 in favor of this PR. Thanks for the discussion! I think we have found a good solution to go forward with.

@mtrezza mtrezza changed the title Generate Typescript types definitions using flow-to-ts && tsc Generate Typescript types definitions using flow-to-ts Apr 12, 2021
@mtrezza mtrezza changed the title Generate Typescript types definitions using flow-to-ts Generate TypeScript definitions with flow-to-ts Apr 12, 2021
@mtrezza mtrezza changed the title Generate TypeScript definitions with flow-to-ts Add TypeScript definitions with flow-to-ts Apr 12, 2021
@dblythy
Copy link
Member

dblythy commented Apr 12, 2021

I'm really looking forward to this!

@sadortun
Copy link
Contributor Author

I'm just leaving this here for future use

Current types in @types/parse:

Server

  • namespace Cloud
  • interface CookieOptions
  • interface HttpResponse
  • interface JobRequest
  • interface Params
  • interface FunctionRequest
  • interface ValidatorField
  • interface ValidatorFields
  • interface Validator
  • interface Cookie
  • interface TriggerRequest
  • interface AfterSaveRequest
  • interface AfterDeleteRequest
  • interface BeforeDeleteRequest
  • interface BeforeSaveRequest
  • interface FileTriggerRequest
  • interface BeforeFindRequest
  • interface AfterFindRequest
  • interface RunOptions
  • interface HTTPOptions

Parse-JS

  • class ACL
  • class File
  • class GeoPoint
  • class Relation
  • class Polygon
  • class Query
  • class LiveQuerySubscription
  • class Config
  • class Schema
  • class Error
  • interface BatchSizeOption
  • interface CascadeSaveOption
  • interface SuccessOption
  • interface ErrorOption
  • interface FullOptions
  • interface RequestOptions
  • interface SuccessFailureOptions
  • interface SignUpOptions
  • interface SessionTokenOption
  • interface WaitOption
  • interface UseMasterKeyOption
  • interface ScopeOptions
  • interface SilentOption
  • interface Pointer
  • interface AuthData
  • interface declaration
  • interface AuthProvider
  • interface BaseAttributes
  • interface CommonAttributes
  • interface JSONBaseAttributes
  • interface Attributes
  • interface Object
  • interface ObjectStatic
  • interface ObjectConstructor
  • interface DestroyOptions
  • interface DestroyAllOptions
  • interface FetchAllOptions
  • interface FetchOptions
  • interface SaveOptions
  • interface SaveAllOptions
  • interface SetOptions
  • interface Installation
  • interface InstallationConstructor
  • interface EachOptions
  • interface CountOptions
  • interface FindOptions
  • interface FirstOptions
  • interface GetOptions
  • interface AggregationOptions
  • interface FullTextOptions
  • interface BatchOptions
  • interface Role
  • interface RoleConstructor
  • interface Session
  • interface SessionConstructor
  • interface User
  • interface UserConstructor
  • interface FieldOptions
  • interface Index
  • interface CLPField
  • interface CLP
  • interface BaseOperation
  • interface Add
  • interface AddUnique
  • interface Increment
  • interface Relation
  • interface Set
  • interface Unset
  • interface PushData

@mtrezza
Copy link
Member

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

We are planning to release the first beta version of Parse Server 5.0 in October 2021.

If a PR contains a breaking change and is not merged before the beta release of Parse Server 5.0, it cannot be merged until the end of 2022. Instead it has to follow the Deprecation Policy and phase-in breaking changes to be merged during the course of 2022.

One of the most voiced community feedbacks was the demand for predictability in breaking changes to make it easy to upgrade Parse Server. We have made a first step towards this by introducing the Deprecation Policy in February 2021 that assists to phase-in breaking changes, giving developers time to adapt. We will follow-up with the introduction of Release Automation and a branch model that will allow breaking changes only with a new major release, scheduled for the beginning of each calendar year.

We understand that some PRs are a long time in the making and we very much appreciate your contribution. We want to make it easy for PRs that contain a breaking change and were created before the introduction of the Deprecation Policy. These PRs can be merged with a breaking change without being phased-in before the beta release of Parse Server 5.0. We are making this exception because we appreciate that this is a time of transition that requires additional effort from contributors to adapt. We encourage everyone to prepare their PRs until the end of September and account for review time and possible adaptions.

If a PR contains a breaking change and should be merged before the beta release, please mention @parse-community/server-maintenance and we will coordinate with you to merge the PR.

Thanks for your contribution and support during this transition to Parse Server release automation!

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.

3 participants