Building TypeScript type definitions, along with some JSDoc #210

gudzpoz · 2023-04-27T10:43:54Z

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.

Possibly due to [TypeScript#51819], the exported declarations are quite incomplete when re-exporting using `module.exports.key = value` syntax. [TypeScript#51819]: microsoft/TypeScript#51819

Attempts to make tsc happy. Possible bugs: - 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

socket-security · 2023-04-27T10:46:05Z

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

daurnimator

Thanks!

Please forgive any stupid review comments, as typescript isn't something I've really used before.

I only got half way through reviewing and my attention faded; so this is only a partial review.

Other misc note: I think I somewhat intended that fengari strings being Uint8Array was meant to be opaque to the user. Is it possible to make the string type distinct Uint8Array-like? or possibly an alias?

src/defs.js

daurnimator · 2023-04-27T12:39:19Z

src/defs.js

 let luastring_indexOf;
 if (typeof (new Uint8Array().indexOf) === "function") {
    luastring_indexOf = function(s, v, i) {
        return s.indexOf(v, i);
    };
 } else {
    /* Browsers that don't support Uint8Array.indexOf seem to allow using Array.indexOf on Uint8Array objects e.g. IE11 */
-    let array_indexOf = [].indexOf;
+    let array_indexOf = [0].indexOf;


Interesting. I'm sorta surprised this works

This is actually because TypeScript treats [] as Array<undefined> and forbids using it as Array<number>. Probably using Array.prototype.indexOf is more understandable.

daurnimator · 2023-04-27T12:43:24Z

src/defs.js

@@ -232,6 +288,7 @@ const from_userstring = function(str) {
            throw new TypeError("expects an array of bytes or javascript string");
        }
    }
+    // @ts-ignore


Why is this needed? / is there some other decorator we could use?

Well, TypeScript is called AnyScript for a reason - it fails to deduce that str has to be Uint8Array. An alternative is to make things more explicit:

const from_userstring = function(str) { - if (!is_luastring(str)) { + if (!(str instanceof Uint8Array)) {

(One may also use // @ts-expect-error, but neither offers a way to specify the suppressed error type.)

daurnimator · 2023-04-27T12:46:59Z

src/defs.js

+    LUA_TSHRSTR:        0,
+    LUA_TLNGSTR:        0,
+    LUA_TNUMFLT:        0,
+    LUA_TNUMINT:        0,
+    LUA_TLCL:           0,
+    LUA_TLCF:           0,
+    LUA_TCCL:           0,


I think this makes the code harder to understand

TypeScript does not track type info of the fields assigned only after object initialization. What about:

const LUA_OK = 0; const LUA_YIELD = 1; const LUA_ERRRUN = 2; const LUA_ERRSYNTAX = 3; const LUA_ERRMEM = 4; const LUA_ERRGCMM = 5; const LUA_ERRERR = 6; const LUA_TSHRSTR = LUA_TSTRING | (0 << 4); /* short strings */ const LUA_TLNGSTR = LUA_TSTRING | (1 << 4); /* long strings */ const LUA_TNUMFLT = LUA_TNUMBER | (0 << 4); /* float numbers */ const LUA_TNUMINT = LUA_TNUMBER | (1 << 4); /* integer numbers */ const LUA_TLCL = LUA_TFUNCTION | (0 << 4); /* Lua closure */ const LUA_TLCF = LUA_TFUNCTION | (1 << 4); /* light C function */ const LUA_TCCL = LUA_TFUNCTION | (2 << 4); /* C closure */ const thread_status = { LUA_OK, LUA_YIELD, LUA_ERRRUN, LUA_ERRSYNTAX, LUA_ERRMEM, LUA_ERRGCMM, LUA_ERRERR, LUA_TSHRSTR, LUA_TLNGSTR, LUA_TNUMFLT, LUA_TNUMINT, LUA_TLCL, LUA_TLCF, LUA_TCCL, };

daurnimator · 2023-04-27T12:47:47Z

src/fengari.js

-module.exports.lua     = lua;
-module.exports.lauxlib = lauxlib;
-module.exports.lualib  = lualib;
+module.exports = exported;


I recall that this caused problems in some ancient node version; or was it an issue with particular minifiers?

It seems to cause problems when there is a cyclic dependency: https://stackoverflow.com/questions/63371870/object-assignmodule-exports-vs-module-exports
Unfortunately, loadlib.js depending on fengari.js creates exactly a dependency cycle. Maybe we can defer requiring fengari.js (which is only used inside a library loading function)?

(I just tried Object.assign and it turns out TypeScript empties all types definitions, leaving only export {};.)

daurnimator · 2023-04-27T12:56:25Z

src/loslib.js

@@ -85,7 +85,8 @@ const setallfields = function(L, time, utc) {
    setfield(L, "month", (utc ? time.getUTCMonth()   : time.getMonth()) + 1);
    setfield(L, "year",  utc ? time.getUTCFullYear() : time.getFullYear());
    setfield(L, "wday",  (utc ? time.getUTCDay()     : time.getDay()) + 1);
-    setfield(L, "yday", Math.floor((time - (new Date(time.getFullYear(), 0, 0 /* shortcut to correct day by one */))) / 86400000));
+    setfield(L, "yday", Math.floor(
+        (time.valueOf() - (new Date(time.getFullYear(), 0, 0 /* shortcut to correct day by one */)).valueOf()) / 86400000));


I'm surprised typescript can't figure out this line without the .valueOf(). It really makes it less readable

Sadly true. I really cannot think of a better way to do this though. (Or maybe just @ts-ignore.)

src/loslib.js

src/lvm.js

gudzpoz · 2023-04-27T15:28:34Z

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.

@ts-ignore

- Define new variables when reusing older ones confuse tsc - Pass buffers to nodejs functions to avoid decoding errors - Refine type info and rid @ts-ignore

rzvxa · 2023-07-14T22:45:23Z

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

rzvxa · 2023-07-15T05:09:18Z

@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.

giann · 2023-07-15T11:34:49Z

@rzvxa Please do 👍

rzvxa · 2023-07-17T20:10:25Z

@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 · 2023-07-18T13:56:27Z

@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.

gudzpoz · 2023-07-21T13:34:51Z

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:

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).
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)
tsc type info generation can be lame at times (examples in the discussions above like Building TypeScript type definitions, along with some JSDoc #210 (comment) and Building TypeScript type definitions, along with some JSDoc #210 (comment) , 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)?

rzvxa · 2023-07-22T15:03:09Z

@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 · 2023-07-22T15:17:19Z

@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 · 2023-07-22T15:18:03Z

@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 · 2023-07-22T15:20:45Z

@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 · 2023-07-22T19:47:41Z

@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!

leopoldhub · 2024-12-01T18:53:36Z

Hi @gudzpoz ,

Is this PR still being worked on?

Same as @rzvxa said, I would also like to use and contribute to this niche but great project.
If there is anything I can do, or you want to delegate some of it, please let me know, I would be more than happy to help.

@gudzpoz

Found via #210 with thanks to @gudzpoz

@gudzpoz

Found via #210 with thanks to @gudzpoz

@gudzpoz

It doesn't take a littleendian argument Found via #210 with thanks to @gudzpoz

daurnimator · 2024-12-02T01:46:36Z

I've cherry picked a few of the fixed into master

leopoldhub · 2024-12-02T07:25:32Z

Thank you very much for your work!
My proposition for the rest of it is still valid, feel free to reach out whenever you want.

gudzpoz · 2024-12-05T17:13:47Z

@leopoldhub No, currently I am not working on this PR and might have lost track of the state of it over the last year... Now that the fixes have been merged, it seems to me that it only needs a bit more fixes to be ready for a second review:

Consider reverting some changes that hamper code readability or performance (mentioned in Building TypeScript type definitions, along with some JSDoc #210 (comment)) (and use @ts-ignore or JSDoc instead)
Check if newer versions of tsc can make some of the workarounds unnecessary

Unfortunately, I haven't touched JS/TS for a while and might need quite some time to pick up this PR again. I've added you as a collaborator to the merging fork, if you are interested, you are welcome to continue the work on this PR!

gudzpoz added 9 commits April 27, 2023 11:59

Generate type definitions with tsc

b627429

src/defs.js: add jsdoc with typing info

40bfd91

src/luaconf.js: add typing info

b400812

src/lapi.js: add type info

d7bc2fe

src/lualib.js: add type info

71d42ec

src/lauxlib.js: add type info

16a4a59

Fix incorrect signatures with tsc checkJs

64aaa89

Workaround possible tsc caveat when re-exporting

477f93f

Possibly due to [TypeScript#51819], the exported declarations are quite incomplete when re-exporting using `module.exports.key = value` syntax. [TypeScript#51819]: microsoft/TypeScript#51819

daurnimator reviewed Apr 27, 2023

View reviewed changes

gudzpoz added 5 commits April 28, 2023 01:34

Reverse several unnecessary changes

7523f53

- Define new variables when reusing older ones confuse tsc - Pass buffers to nodejs functions to avoid decoding errors - Refine type info and rid @ts-ignore

Test type info with test files

81472fb

Add a os.rename/remove test

8eec9d5

Use LuaString as a type alias for Uint8Array

4e3dc6a

Ask tsc to merge all d.ts files into one

a6476ff

daurnimator added a commit that referenced this pull request Dec 2, 2024

src/ldump.js: fix incorrect field in constructor

8df9dde

Found via #210 with thanks to @gudzpoz

daurnimator added a commit that referenced this pull request Dec 2, 2024

src/loadlib.js: fix error message construction

4d85f25

Found via #210 with thanks to @gudzpoz

daurnimator added a commit that referenced this pull request Dec 2, 2024

src/lstrlib.js: remove extraneous argument to setUint8

6a89083

It doesn't take a littleendian argument Found via #210 with thanks to @gudzpoz

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Building TypeScript type definitions, along with some JSDoc #210

Building TypeScript type definitions, along with some JSDoc #210

gudzpoz commented Apr 27, 2023 •

edited

Loading

socket-security bot commented Apr 27, 2023

daurnimator left a comment

daurnimator Apr 27, 2023

gudzpoz Apr 27, 2023

daurnimator Apr 27, 2023

gudzpoz Apr 27, 2023 •

edited

Loading

daurnimator Apr 27, 2023

gudzpoz Apr 27, 2023 •

edited

Loading

daurnimator Apr 27, 2023

gudzpoz Apr 27, 2023 •

edited

Loading

daurnimator Apr 27, 2023

gudzpoz Apr 27, 2023

gudzpoz commented Apr 27, 2023

rzvxa commented Jul 14, 2023

rzvxa commented Jul 15, 2023 •

edited

Loading

giann commented Jul 15, 2023 •

edited

Loading

rzvxa commented Jul 17, 2023

rzvxa commented Jul 18, 2023

gudzpoz commented Jul 21, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023 •

edited

Loading

leopoldhub commented Dec 1, 2024

daurnimator commented Dec 2, 2024

leopoldhub commented Dec 2, 2024

gudzpoz commented Dec 5, 2024

Building TypeScript type definitions, along with some JSDoc #210

Are you sure you want to change the base?

Building TypeScript type definitions, along with some JSDoc #210

Conversation

gudzpoz commented Apr 27, 2023 • edited Loading

socket-security bot commented Apr 27, 2023

daurnimator left a comment

Choose a reason for hiding this comment

daurnimator Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz Apr 27, 2023

Choose a reason for hiding this comment

daurnimator Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

daurnimator Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

daurnimator Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

daurnimator Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz Apr 27, 2023

Choose a reason for hiding this comment

gudzpoz commented Apr 27, 2023

rzvxa commented Jul 14, 2023

rzvxa commented Jul 15, 2023 • edited Loading

giann commented Jul 15, 2023 • edited Loading

rzvxa commented Jul 17, 2023

rzvxa commented Jul 18, 2023

gudzpoz commented Jul 21, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023

rzvxa commented Jul 22, 2023 • edited Loading

leopoldhub commented Dec 1, 2024

daurnimator commented Dec 2, 2024

leopoldhub commented Dec 2, 2024

gudzpoz commented Dec 5, 2024

gudzpoz commented Apr 27, 2023 •

edited

Loading

gudzpoz Apr 27, 2023 •

edited

Loading

gudzpoz Apr 27, 2023 •

edited

Loading

gudzpoz Apr 27, 2023 •

edited

Loading

rzvxa commented Jul 15, 2023 •

edited

Loading

giann commented Jul 15, 2023 •

edited

Loading

rzvxa commented Jul 22, 2023 •

edited

Loading