qooxdoo / qooxdoo

qooxdoo - Universal JavaScript Framework
http://qooxdoo.org
Other
764 stars 259 forks source link

Better Typescript generation #10646

Closed johnspackman closed 1 month ago

johnspackman commented 4 months ago

Previously, the compiler only output typescript definitions as it compiled, but this caused problems because not all targets support all javascript files (eg browser vs node code)

This PR moves the meta data collection into a separate parser which can scan every class file in every library, and generate meta data and typescript.

All you now need to do is:

qx compile --typescript

It works with the --watch argument too, so you can continuously update the .d.ts file.

The output has changed - instead of adding the meta data into the target's transpiled/ directory, there is now a separate compiled/meta directory

This will require some changes to the API viewer, I'll work on that shortly

WillsterJohnsonAtZenesis commented 4 months ago

Let's quickly address why so many seemingly unrelated files have changes;

A big priority for this PR is to ensure that the generated typescript declaration file is useable and error-free. This meant a large chunk of the existing JSDoc had to be changed one way or another.

Thankfully though a lot of the problem areas are part of a quite uniform standard of sorts, meaning the source code can remain untouched and the TypescriptWriter can fix the incompatibilties at compile time.

There were of course some areas which don't fit into that category, these are the majority of the files which have changed. The changes are only in the JSDoc type information, and as far as possible the types are directly compatible and (hopefully) clear and obvious.

There were also some areas, particularly in the later commits, where this generated dts file caught type errors or bugs in some parts of the code. These have been resolved to be as type-safe as possible and shouldn't have any notable effects at runtime.

The end result of this is that running qx compile --typescript will now produce a .d.ts file at the chosen location (default compiled/qooxdoo.d.ts) with 0 errors in qx.*, as well as a source/global.d.ts with a reference directive to the above dts file for better support from tooling and text editors such as VSCode.

johnspackman commented 4 months ago

Got in directory compiled/meta a file with name "undefined.json". I used the command "compile" with argument "--meta".

fixed 👍

goldim commented 4 months ago

Got in directory compiled/meta a file with name "undefined.json". I used the command "compile" with argument "--meta".

fixed 👍

Great! Today-tomorrow I will check it out.

goldim commented 4 months ago

Got also two warnings from qx framework during compile -T:

Ignoring class 'qx.test.ui.core.Theme' in file '../../../../../../../qx/qooxdoo/source/class/qx/test/ui/core/Appearance.js' because a class, mixin, or interface was already found in this file. Ignoring class 'qx.test.ui.core.Test' in file '../../../../../../../qx/qooxdoo/source/class/qx/test/ui/core/Appearance.js' because a class, mixin, or interface was already found in this file.

And got this type of warning for my app for the second class in file. Is it normal case?

WillsterJohnsonAtZenesis commented 4 months ago

I got the next error after qx compile --typescript in qooxdoo.d.ts:

"}" expected

Looks like last package/class is not closed up.

This is due to some apps/libs having their alphabetical last class within a package (like qxl.Foo) and some not, specifically in qooxdoo's lib there's qxWeb after the last qx.*. This should be resolved now.

Got also two warnings from qx framework during compile -T:

Ignoring class 'qx.test.ui.core.Theme' in file '../../../../../../../qx/qooxdoo/source/class/qx/test/ui/core/Appearance.js' because a class, mixin, or interface was already found in this file. Ignoring class 'qx.test.ui.core.Test' in file '../../../../../../../qx/qooxdoo/source/class/qx/test/ui/core/Appearance.js' because a class, mixin, or interface was already found in this file.

And got this type of warning for my app for the second class in file. Is it normal case?

You can expect multiple top-level classes in a single file to not all be processed - the metadata parser will only process one class. Currently, that's the first one which is defined, all others produce the error. As for the error occuring on qooxdoo's lib, that's been resolved now.

The other change requests are also resolved now.

hkollmann commented 4 months ago

Error: error: Expected { after 'if' condition (curly) at source/class/qx/tool/compiler/targets/TypeScriptWriter.js:148:28: 146 | await this.writeClass(metaData, declared); 147 | }

148 | if (lastPackageName) this.write("}\n"); | ^ 149 | await this.close(); 150 | }, 151 |

There is a lint issue left.

hkollmann commented 3 months ago
     @goldim: Could you have a look again? Thanks!
goldim commented 3 months ago
     @goldim: Could you have a look again? Thanks!

I will check it out near soon. There are two notices which were ignored but they are just not so important, I guess.

johnspackman commented 3 months ago

sorry, i cherry picked into the wrong branch yesterday! I'll fix this later this morning

hkollmann commented 3 months ago

@goldim : Is your problem fixed?

goldim commented 3 months ago

@goldim : Is your problem fixed?

Check it out today and reply

WillsterJohnsonAtZenesis commented 3 months ago

There are some errors in output typescript file. For example interface of method placeToWidget returns void but implementation returns boolean. They are not compatible. Is it fine?

Still exists. And didn't get answer. Also whole file is red in VS Code. Also often behold the next error:

This member cannot have an 'override' modifier because it is not declared in the base class 'Message'

Getting rid of override seems like solves the problem but I don't know. If these errors are unimportant just tell me I will pass the PR.

I've written a fix for the issue with override, there should no longer be any cases where the keyword is used incorrectly.

Errors where the type definition does not match the actual returned data are down to invalid or incomplete JSDoc. It's quite easy to stumble into one of these cases while working, but actually identifying all of them would need a custom type parser which understands Qooxdoo's classes.

I've also fixed a few cases where the JSDoc types were simply incorrect, such as putting Error in the @returns when it should either be in @throws or simply omitted.

All that's left are 8 errors which have no 'way out'. These remain because they are overrides of methods on the native Array class which have very different type signatures to the original, eg qx.ui.website.Table.sort takes two strings while it's super Array.sort takes a callback. @johnspackman tells me qxWeb is deprecated, so ignoring these errors is fine.

goldim commented 3 months ago

@WillsterJohnsonAtZenesis I've checked result of old generation and there are also errors. In your current generation I see with VS Code most of left errors left are result type: function and array (underlined with red color). First one could be fixed with Function and array with []. Is it possible to do? Would you describe left errors which are "no way" or how to see them? About array seems like I understand you.

goldim commented 2 months ago

@WillsterJohnsonAtZenesis I looked at your last changes and want to say that just Array is shown as invalid. Function is right. I mean that there are no difference between array and Array.

p9malino26 commented 2 months ago

Those JSON files in compiled/meta do not have inherited members in them, but old files in compiled/source/transpiled used to. Do we want the new JSON files in meta to include inherited members as well?

johnspackman commented 2 months ago

Those JSON files in compiled/meta do not have inherited members in them, but old files in compiled/source/transpiled used to. Do we want the new JSON files in meta to include inherited members as well?

I dont think so - my reasoning was that a lot of work was going into producing information which was redundant because it already exists, and would only be necessary in a specific application ... it also meant that if the app that reads meta data only wants to know what is implemented in a class, it would have to exclude the inherited methods. The meta data as it stands now accurately reflects what is in the class.

goldim commented 2 months ago

Did you finish with changes may I check it? There are merge conflicts btw.

johnspackman commented 1 month ago

@goldim sorry for the delay, I think I've just fixed the conflicts and if the tests pass you should be ready to test

goldim commented 1 month ago

@goldim sorry for the delay, I think I've just fixed the conflicts and if the tests pass you should be ready to test

@johnspackman thank you for the response. I will have a look at it but I don't have any good showcases and testcases for TS in other words I don't use it. For me will be enough if anything is not broken. And hope if something with TS will be fixed during using it. Also would like to know about gitter. I've noticed you don't read it or u are just in "hidden" mode?

johnspackman commented 1 month ago

This should fix just about everything, there were some issues with consistency in Qooxdoo classes but the TS seems quite tolerant. The meta data parser is really a first stage to having a two-pass compiler, and will allow the compiler to do a lot more detailed type checking as it compiles - but for existing TS users, this should represent a big step forward in getting integration with their IDE (@WillsterJohnsonAtZenesis and @p9malino26 use it)

johnspackman commented 1 month ago

PS - Gitter! Wow, I'm so used to it running in the background I'd forgotten to start it for ages - i'll check in now

goldim commented 1 month ago

@johnspackman it is about IDE integration which will help IDE to autocomplete code, isn't it?

johnspackman commented 1 month ago

it is about IDE integration which will help IDE to autocomplete code, isn't it?

Yes - this does not generate actual typescript, it outputs a "*.d.ts" file which just contains the definitions - ie it describes your classes in a form that TS can understand (and therefore vscode) so that you get type completion and other goodies.

The only thing you need is a tsconfig.json file in your project and to configure the generation and you should be set

hkollmann commented 1 month ago

@goldim : Could you review again please?

johnspackman commented 1 month ago

this should be ready to merge (subject to the tests passing of course :) )

The change in the last commit is to make it so that meta is always generated, and the config file collates settings together into an object (it also fixes a bug to handle annotations properly).

The reason for meta being "always on" is that there will be another PR shortly where the meta data becomes the "first pass" of the compiler, so that we can get better warnings, set the groundwork for adding type safety, and simplifying various parts of the compiler