fengari
fengari copied to clipboard
Building TypeScript type definitions, along with some JSDoc
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.
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 |
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 @daurnimator I was wondering what is the state of this PR. When does it get merged into the master?
@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 Please do 👍
@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.
@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.
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
- Type info generation with
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)
- Either
- Despite the fact that
-
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 wanttsc
to generate correct type info, you will need to find ways to maketsc
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 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)
@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
@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 asi + 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
@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 asi + 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
@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!