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

Switches client code from JS to TS #3803

Open
wants to merge 72 commits into
base: main
Choose a base branch
from

Conversation

walmazacn
Copy link
Contributor

Description

Changes proposed in this pull request:

  • switches client code from JS to TS

Related issue(s)

Resolves #3791

@walmazacn walmazacn marked this pull request as ready for review July 5, 2024 11:30
@JohannesDoberer JohannesDoberer self-assigned this Jul 8, 2024
@ndricimrr ndricimrr self-assigned this Jul 8, 2024
client/luigi-client.d.ts Show resolved Hide resolved
client/luigi-client.d.ts Outdated Show resolved Hide resolved
client/luigi-client.d.ts Outdated Show resolved Hide resolved
client/luigi-element.d.ts Show resolved Hide resolved
client/src/lifecycleManager.ts Show resolved Hide resolved
client/src/lifecycleManager.ts Outdated Show resolved Hide resolved
client/src/linkManager.ts Outdated Show resolved Hide resolved
client/src/luigi-element.ts Show resolved Hide resolved
test/e2e-angular.sh Outdated Show resolved Hide resolved
scripts/package.json Outdated Show resolved Hide resolved
scripts/package.json Outdated Show resolved Hide resolved
Comment on lines 13 to 14
"docu:client:generate:section": "documentation readme ../client/luigi-client.d.ts --parse-extension ts -f md --readme-file=../docs/luigi-client-api.md --section=\"API Reference\" --markdown-toc=false --quiet --github false",
"docu:client:validate": "documentation lint ../client/luigi-client.d.ts",
Copy link
Contributor

@ndricimrr ndricimrr Aug 21, 2024

Choose a reason for hiding this comment

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

I think the old ../client/src/luigi-client.js (100+ lines) was used to generate the luigi-client-api.md (1400+ lines) sections.

It seems like documentation.js generates .md file by recursively going into the code, starting with imports (in the strict order they were written), which explains why 1400+ lines of .md.

So if you give it another file with different types of imports and different structure (luigi-client.d.ts) it will completely change how the resulting .md looks like.

I don't think we can ever review that .md file if we completely change the imports, and we'll never know if we missed/lost some docu in the process, so the best bet would be to keep the imports in src/luigi-client.ts exactly the same as luigi-client.js , and push for 'any' typings in there where needed , so don't mess up the docu 🤔 .

I think the luigi-client.d.ts was created with the purpose of having typings for Typescript users because we only had luigi-client**.JS** . We would have wanted to have an auto .d.ts generation at some point to avoid having to maintain a manually written extra .d.ts typings file, so there goes luigi-client.d.ts which might be a bit chaotic because we only wanted to export types not use it for documentation.

Which is why I think luigi-client.d.ts shouldn't be used in this case to generate docu but rather the new luigi-client.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be some issue with parsing TS files that comes from documentation lib - according to their changelog it was fixed in version 14.0.2:
https://github.com/documentationjs/documentation/blob/master/CHANGELOG.md#bug-fixes-1

I tested the latest version locally and it can handle TS imports correctly - but unfortunatelly many newlines are removed and output file is unreadable :( Similar issue is already reported:
documentationjs/documentation#1595

I can see some possible solutions here, but none of them is perfect:

  1. fork documentation lib from lower version and try to fix TS issue on our own
  2. freeze building of documentation file for client until TS issues are fixed - API is not updated so often

@walmazacn walmazacn added the WIP Work in progress label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch all client code from JS to Typescript
3 participants