-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I had a look at the errors, and what it shows is even if The most suitable way forward, would probably be to complete type definitions in current I dont think there is too much drawbacks in adding types definitions to existing code. But this is a long-term effort toward Typescript |
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? |
Ok perfect! #7335 FTW ! |
You mean the errors occur because some parts of the code were not flow type annotated? |
Yes |
The flow-to-ts module has no option to simply ignore non-annotated code? |
When you run TSC after that, it generate errors because of missing annotations, or use of You can either add types now in flow, and re-run 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 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. |
Oh, the errors do not come from |
Quick comparaisonCode generated from your #7335 branch using JSDocYou can see that about everything is /// <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
|
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 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 |
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)?
|
Correct me if in wrong, but from what I've seen, #7335 does generate JSDoc and
#7337 on the other hand can generate "good" quality Its also possible to move toward (3) later next year. The real magic behind
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
No, that's why Unless you do really Wierd things in So if you update the current (*) I haven't do through ALL of
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. |
Generated by 🚫 dangerJS |
It uses the existing types in JSDoc to generate TS. For example in
Generate JSDoc? Or do you mean it just passes the JSDoc from the
Sounds good, so the process would be:
Something like that? |
Hehehe, yes :)
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.
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
Thats ensure the user uninstall 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.
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
🚀🚀🚀 Yeahh 🚀🚀🚀 !! |
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! |
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
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 )
So the good thing with this way to do things is that you can decide when you do the transition from 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 :
|
Maybe something to consider, prettier is pretty opinionated.
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. |
I'm really looking forward to this! |
I'm just leaving this here for future use Current types in Server
Parse-JS
|
|
New Pull Request Checklist
Issue Description
Tests with
flow-to-ts
.You can test out by running
This run well, except
npx typescript
...Related issue: #7334 #7335 #7336
Approach
TODOs before merging