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

fix(scripting/lua): support exported MRVs #681

Closed
wants to merge 1 commit into from

Conversation

lze3
Copy link
Contributor

@lze3 lze3 commented Mar 10, 2021

I believe this is quite hacky and could be done cleaner, however, this is the only approach I could think of.

The issue is that MRVs aren't properly handled with the Lua ScRT because of it only returning the second parameter of pcall, which does not take into account MRVs. The only way to actually ensure that all MRVs are a gotten is through packing the pcall return values and handling it's return as normal, afterward we just check if the pack is >2 (has 2+ MRVs) and if so, we just remove 1st index and unpack. Othewise, just return the second retval of the pcall (first is status of the pcall).

Some tests for ensuring no regressions are introduced with these changes:

Lua

Setting in Lua and also getting

exports('test_exp', function(opts)
    if opts and opts.mrv then
        return 4, 3, 5, 7, 9
    end

    return 155
end)

local doTest = 1

local test_exp = function(...) return exports[GetCurrentResourceName()]:test_exp(...) end
local log = function(a) print("^3" .. a) end
local newLine = function() print() end

if doTest then
    newLine()
    log('Lua Test')
    log('Description: ^4Ensure MRVs are handled with exports from the Lua ScRT')
    log('Expect: ^4if ret val == 1 then return it, otherwise return all of them\n')
    do
        log('Returning 1 value:')
        local ret = test_exp()
        log(string.format('- %s | %s', ret, type(ret)))
    end

    newLine()
    do
        local ret, any, other, value, nein = test_exp({ mrv = true })
        log('Returning 5 values:')
        log(string.format('- %s | type: %s', ret, type(ret)))
        print(ret, any, other, value, nein)
    end
end

JavaScript

Only getting in JavaScript

JavaScript behaviour of getting Lua export MRVs was always just packing into an array.

const { test_exp } = exports['test-lua'];

const doTest = 1;
const log = (a) => console.log(`^3${a}`);
const newLine = () => console.log();

if (doTest) {
    newLine();
    log('JavaScript Test');
    log('Description: ^4Ensuring no regressions are made with exporting any amount of values')
    log('Expect: ^4if ret val == 1 then return it, otherwise pack as an array\n');
    {
        log('Returning 1 value:');
        const ret = test_exp();
        log(`- ${ret} | ${typeof ret}`);
    }

    newLine();
    {
        const ret = test_exp({ mrv: true });
        const { length } = ret;
        log(`Returning ${length !== 0 && !length ? new Error('bad len') : length} values:`);
        log(`- ${ret} | type: ${typeof ret} | len: ${length}`);
    }
    newLine();
}

Expected output from running the above code:

@blattersturm
Copy link
Collaborator

This is not the way I was hoping for this to be implemented. I did mention vararg functions in the discussion earlier, that would make more sense than making and manipulating a temporary table.

@lze3 lze3 closed this Mar 10, 2021
@lze3 lze3 deleted the master branch March 10, 2021 15:07
@blattersturm
Copy link
Collaborator

...?

@lze3 lze3 restored the master branch March 10, 2021 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants