ceifa / wasmoon

A real lua 5.4 VM with JS bindings made with webassembly
MIT License
449 stars 29 forks source link

Default null to nil when injectObjects is not set #112

Closed gudzpoz closed 3 months ago

gudzpoz commented 3 months ago

The PR #101 handles null values only when injectObjects is set to true. When injectObjects is undefined or false, null values are not handled at all, somehow triggering a TypeError in PromiseTypeExtension or otherwise a The type 'object' is not supported by Lua error.

const { LuaFactory } = require('wasmoon')
const L = await factory.createEngine({ injectObjects: false })
L.global.pushValue(null) // TypeError here
Uncaught TypeError: Cannot read properties of null (reading 'then')
    at PromiseTypeExtension.pushValue (.../node_modules/.pnpm/wasmoon@1.16.0/node_modules/wasmoon/dist/index.js:916:102)
    at .../node_modules/.pnpm/wasmoon@1.16.0/node_modules/wasmoon/dist/index.js:244:82
    at Array.find (<anonymous>)
    at Global.pushValue (.../node_modules/.pnpm/wasmoon@1.16.0/node_modules/wasmoon/dist/index.js:244:46)

This PR adds a simple DefaultNullTypeExtension for cases when injectObjects is false.

fixes #104

gudzpoz commented 3 months ago

Thanks for the suggestion! It occurs to me that default null values can be handled in type-extensions/table.ts instead (as is done in the latest commit). Or do you prefer it to be handled in a standalone handler?

By the way, I can't think of a good way to move the changes into thread.ts. Do you mean something like this?

diff --git a/src/thread.ts b/src/thread.ts
index 12e26f4..53387e0 100755
--- a/src/thread.ts
+++ b/src/thread.ts
@@ -216,6 +216,12 @@ export default class Thread {
             case 'boolean':
                 this.lua.lua_pushboolean(this.address, target ? 1 : 0)
                 break
+            case 'object':
+                if (target === null && !((this.parent ?? this) as unknown as Global).injectObject) {
+                    this.lua.lua_pushnil(this.address)
+                    break
+                }
+            // Otherwise, fall through
             default:
                 if (!this.typeExtensions.find((wrapper) => wrapper.extension.pushValue(this, decoratedValue, userdata))) {
                     throw new Error(`The type '${typeof target}' is not supported by Lua`)
tims-bsquare commented 3 months ago

Thanks for the suggestion! It occurs to me that default null values can be handled in type-extensions/table.ts instead (as is done in the latest commit). Or do you prefer it to be handled in a standalone handler?

By the way, I can't think of a good way to move the changes into thread.ts. Do you mean something like this?

diff --git a/src/thread.ts b/src/thread.ts
index 12e26f4..53387e0 100755
--- a/src/thread.ts
+++ b/src/thread.ts
@@ -216,6 +216,12 @@ export default class Thread {
             case 'boolean':
                 this.lua.lua_pushboolean(this.address, target ? 1 : 0)
                 break
+            case 'object':
+                if (target === null && !((this.parent ?? this) as unknown as Global).injectObject) {
+                    this.lua.lua_pushnil(this.address)
+                    break
+                }
+            // Otherwise, fall through
             default:
                 if (!this.typeExtensions.find((wrapper) => wrapper.extension.pushValue(this, decoratedValue, userdata))) {
                     throw new Error(`The type '${typeof target}' is not supported by Lua`)

Personally I think it should be handled in thread.ts with the other primitives, definitely not in the table extension though, it may be an object but not one with keys. The downside of that being you'd still need to allow extensions to overwrite null.

In thread.ts I think what you'd have to do is in the default case when no extensions are found then if the value is null then push nil and the same in reverse for the get.

To be clear though it may be better as a type extension separate from the current null and table types. I only suggest thread.ts because there's already some null/undefined handling in there.

ceifa commented 3 months ago

LGTM