Skip to content
This repository was archived by the owner on Aug 9, 2024. It is now read-only.

Conversation

@nullsoepic
Copy link
Contributor

This PR adds:

  • Native JSON functions
  • Time function
  • Require implementation
  • Handling of non-JSON responses with fetch

JSON

Can be just used as json.stringify() and json.parse(), no need to require/import anything

Time

Used as time(), returns seconds instead of millis like the standard lua os.time()

Require

Fetches, loads and executes some lua code from a url, and returns the output

Fetch

Fetch now supports responses that do not return JSON, for backwards compatibility and simplicity, a table will be returned if the response is valid JSON

@jamiw1
Copy link

jamiw1 commented Jun 3, 2024

require should work with local/relative paths in addition to urls, and Time() should absolutely use milliseconds (preferably unix time) instead of just seconds

@nullsoepic
Copy link
Contributor Author

require should work with local/relative paths in addition to urls, and Time() should absolutely use milliseconds (preferably unix time) instead of just seconds

I attempted to support relative paths (03006c8) but had a bunch of issues, still not sure how that could be implemented.

time already uses unix time, the reason for using seconds is compatibility with standard lua. We could have a argument/seperate function to get milliseconds

@GStudiosX2
Copy link
Contributor

Fetch already supported non Json ?

@nullsoepic
Copy link
Contributor Author

My bad, looks like it does work, had an issue where github raw urls would just return a table with status 200 and no content so I assumed it just was not supported without looking at it enough.

Though the current implementation is a bit inconsistent, requests that return json will give you the json as a table, where as other requests that don't return json will give you a table with the raw content and status.

My implementation just gives you a table if its valid json, and a string with the raw response if it isn't.
Looking at it now, it might be better to return a table with status and content in both cases and just have the content vary?

@face-hh
Copy link
Owner

face-hh commented Jun 4, 2024

Should this comply with the sandbox change in #121?

Quoting the code change:

-    let ok = lua.load(luacode).eval::<LuaMultiValue>();
+    if let Err(e) = lua.sandbox(true) {
+        lualog!("error", format!("failed to enable sandbox: {}", e));
+        Err(LuaError::runtime("failed to enable sandbox"))
+    } else {
+        let ok = lua.load(luacode).eval::<LuaMultiValue>();
+    }

Also, os.time() has been added by #121 so there's no need for a dedicated time function.

@nullsoepic
Copy link
Contributor Author

Considering #121 has been merged, It is probably a good idea to do so, will update asap. Still need an opinion on the changes to fetch and suggestion from the previous comment

@face-hh
Copy link
Owner

face-hh commented Jun 4, 2024

it might be better to return a table with status and content in both cases and just have the content vary

I agree with this but it will break current websites, I propose to revert the fetch behavior to the old one

@nullsoepic
Copy link
Contributor Author

Alright fetch will be reverted.
Just to note, require still has to be given a http(s):// url as I could not get the taburl in the context of the function (any suggestions are appreciated). No ideas on how that could be properly implemented atm, will most likely remain as is for now.

@face-hh face-hh changed the title [napture] feat: Native JSON functions, time(), require(), support non-JSON responses with fetch() [napture] feat: Native JSON functions, require() Jun 4, 2024
@face-hh
Copy link
Owner

face-hh commented Jun 4, 2024

It doesn't seem like you've reverted the fetch implementation as I'm getting this error (which doesn't occur on the master branch):

runtime error: [string "src\b9\lua.rs:445:22"]:57: attempt to concatenate string with nil

I've tested the JSON utility and it seems to work, I'm gonna check out the require feature in a bit

@face-hh
Copy link
Owner

face-hh commented Jun 4, 2024

Alright, everything seems to work

@face-hh face-hh merged commit fae3612 into face-hh:master Jun 4, 2024
@hexxt-git
Copy link

epic

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants