fengari icon indicating copy to clipboard operation
fengari copied to clipboard

Building TypeScript type definitions, along with some JSDoc

Open gudzpoz opened this issue 1 year ago • 13 comments

This PR attempts to address #135 by generating .d.ts files from .js directly. Some JSDoc is added to provide better type hints.

Most of the changes are just JSDoc comments, but running tsc --checkJs does reveal some bugs (or maybe not, I don't know):

  • llimits.js: lua_assert is supplied two params in several places, while it declares only one,
  • lapi.js: there is no ts.value but ts.realstring
  • liolib.js: "19.x.x" > 6 is always false
  • loadlib.js: not lua_pushstring but lua_pushfstring
  • loslib.js: Uint8Arrays instead of strings passed to nodejs functions
  • lstrlib.js: wrong args to luaL_argcheck
  • ltm.js: extraneous new

There are some other code changes just to make tsc happy, and they should not incur any behavioral changes.

This PR can be partially tested with tsc --checkJs, which will also run on npm run test.

P.S. I am not that sure about the correctness of the JSDoc though, which was initially generated with a script scraping the C API.

gudzpoz avatar Apr 27 '23 10:04 gudzpoz

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] None +0 typescript-bot

socket-security[bot] avatar Apr 27 '23 10:04 socket-security[bot]

Using @typedef {Uint8Array} LuaString in JSDoc creates an alias, and it seems TypeScript also supports importing JSDoc aliases from other files. I'll try to replace Uint8Array annotations with the alias.

gudzpoz avatar Apr 27 '23 15:04 gudzpoz

@gudzpoz @daurnimator I was wondering what is the state of this PR. When does it get merged into the master?

rzvxa avatar Jul 14 '23 22:07 rzvxa

@gudzpoz @daurnimator I would like to contribute to this PR if there is anything pending it. I'm planning to use Fengari in my open-source project in order to make plugins for it. Fengari seems more maintained than the node-luajit library which hasn't seen an update in the past 3 years, And also it's written in js and I can easily deploy it on the web (right now the project is electron based). I've had quite an R&D on using eval safe and sandboxing it, But there is always a way to escape, Using lua considering my background in the game industry and loads of experience with Lua C implementation seems just right for this project. I can help with both the type definitions and project maintenance from now going onwards.

rzvxa avatar Jul 15 '23 05:07 rzvxa

@rzvxa Please do 👍

giann avatar Jul 15 '23 11:07 giann

@giann Thanks for your replay, I've forked @gudzpoz 's fork to start working on the type definitions. The first thing I did was run tests, But there is a failing test at test/loslib.test.js, Is that also failing in the main project? or has something broke in this fork? I will investigate more about it and let you know.

rzvxa avatar Jul 17 '23 20:07 rzvxa

@giann @daurnimator I have errors in tests, even in the master branch of Fengari. Is something wrong with my setup? If not, please let me know if you know about this failing test.

rzvxa avatar Jul 18 '23 13:07 rzvxa

IIRC, I was waiting for a full review before deciding on what to do next.

To summarize things a bit, there are a few things that we need to finalize in this PR:

  1. Whether to split this PR into several:

    This PR contains:

    • JSDoc containing both type info and documentation (which I am still a bit unsure about)
    • Build script changes:
      • Type info generation with tsc
      • Linting with tsc with type info (which actually found some bugs)
        • And thus the bug fixes
        • and a couple of workarounds due to limitations in Typescript type deduction

    Personally I feel like at least putting those bug fixes into separate PRs (but I am being really lazy).

  2. Enabling tsc linting requires bringing in several workarounds. As @daurnimator pointed out in the comments, they really hamper code readability and, at times, performance.

    • Despite the fact that tsc actually found some bugs, we might consider disabling linting (at least for now) altogether.
    • Otherwise, we will need to decide upon the better (if not best) workarounds among the possible ways to make tsc satisfied:
      • Either @ts-ignore comments
      • Or use matching types (which can have performance impact)
  3. tsc type info generation can be lame at times (examples in the discussions above like https://github.com/fengari-lua/fengari/pull/210#discussion_r1179249161 and https://github.com/fengari-lua/fengari/pull/210#discussion_r1179401184 , which brings even more workarounds. Unlike the linting workarounds (for which @ts-ignore always works), if you want tsc to generate correct type info, you will need to find ways to make tsc happy.


Also, @rzvxa if you are getting errors, would you please at least attach some of the test output (or, even better, reproducing steps)?

gudzpoz avatar Jul 21 '23 13:07 gudzpoz

@gudzpoz Failed test also fails in the master branch of main project, So it has noting to do with you changes and bug fixes.

 FAIL  test/loslib.test.js
  ● os.getenv

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      159 |     }
      160 |
    > 161 |     expect(lua.lua_isstring(L, -1)).toBe(true);
          |                                     ^
      162 | });
      163 |

      at Object.toBe (test/loslib.test.js:161:37)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

rzvxa avatar Jul 22 '23 15:07 rzvxa

@gudzpoz I think it's better for you to move all of the bug fixes to their own PRs but I think we can keep both JSDocs and typescript definition together in this PR.

Some of the TS warnings can be totally ignored, They are there to stop the developer from shooting themselves in the leg, But for example over here we can leverage typescript instead of actually parsing the number from boolean or use ternary operations. i + (b as unknown as number) by doing this, output javascript would be same as i + b and also we are making TS happy, I've been a C# developer for a long time, I'm used to these type jugglingXD

rzvxa avatar Jul 22 '23 15:07 rzvxa

@gudzpoz I think it's better for you to move all of the bug fixes to their own PRs but I think we can keep both JSDocs and typescript definition together in this PR.

Some of the TS warnings can be totally ignored, They are there to stop the developer from shooting themselves in the leg, But for example over here we can leverage typescript instead of actually parsing the number from boolean or use ternary operations. i + (b as unknown as number) by doing this, output javascript would be same as i + b and also we are making TS happy, I've been a C# developer for a long time, I'm used to these type jugglingXD

https://jsfiddle.net/xdueqj92/1/ I've also forked your benchmark added this new way to the suite

rzvxa avatar Jul 22 '23 15:07 rzvxa

@gudzpoz I think it's better for you to move all of the bug fixes to their own PRs but I think we can keep both JSDocs and typescript definition together in this PR.

Some of the TS warnings can be totally ignored, They are there to stop the developer from shooting themselves in the leg, But for example over here we can leverage typescript instead of actually parsing the number from boolean or use ternary operations. i + (b as unknown as number) by doing this, output javascript would be same as i + b and also we are making TS happy, I've been a C# developer for a long time, I'm used to these type jugglingXD

https://docs.joshuatz.com/cheatsheets/js/jsdoc/#casting-and-type-coercion-in-jsdoc this workaround is also working in JSDoc

rzvxa avatar Jul 22 '23 15:07 rzvxa

@gudzpoz I would love to help with documentation and typing. I want this project to have types and get some maintenance so I can easily use it in my open-source project with no worries. The only fair thing for me to do is to contribute to this project, for myself and anyone who wants to have access to Lua in the JS environment. It's a niche community but I'm sure some projects would love to have a stable lua implementation in Javascript.

If you don't mind, I can collaborate with you on this PR and every other bug fix that comes up in this process. For documentation, we can base our documentation on the Lua project itself.

After this PR we can consider working together on the garbage collection implementation which is currently missing. I can start helping if you delegate some of the remaining tasks to me, You can even give me access to your own fork so we can work on the same repository instead of linking different PRs together. Sir you've done a lot, Please don't give up on this PR and keep on finishing it.

Thanks!

rzvxa avatar Jul 22 '23 19:07 rzvxa