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

Add polling #136

Closed
wants to merge 17 commits into from
Closed

Add polling #136

wants to merge 17 commits into from

Conversation

Ketasaja
Copy link
Contributor

@Ketasaja Ketasaja commented Sep 24, 2024

  • New option call_default.
  • New EvCall:Polling variant.
  • Explicit iteration exists because type inference for loop variables with generalized iteration isn't stable yet until the new solver releases.

Copy link
Collaborator

@sasial-dev sasial-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!
I started by reviewing general code stuff. I'll let @jackdotink review first and then I'll go through and test the output locally.

docs/config/options.md Outdated Show resolved Hide resolved
zap/src/output/luau/client.rs Outdated Show resolved Hide resolved
@Ketasaja
Copy link
Contributor Author

Ketasaja commented Sep 25, 2024

Instead of adding a new event field I added EvCall::Polling.

I need to make the global option configure the default EvCall, update the docs, fix TypeScript polling, and add error handling for when there's no call specified.

@Ketasaja Ketasaja marked this pull request as draft September 25, 2024 04:16
@sasial-dev sasial-dev self-assigned this Sep 25, 2024
@Ketasaja Ketasaja marked this pull request as ready for review September 26, 2024 07:44
@Ketasaja Ketasaja requested a review from sasial-dev September 26, 2024 07:47
@Ketasaja
Copy link
Contributor Author

I think in a rewrite or in the future in general, Zap should report an error and exit immediately when one is encountered.

@sasial-dev
Copy link
Collaborator

The reason why we did that is for error recovery I think. It would make it much eaisier for stuff like building an LSP etc

zap/src/config.rs Outdated Show resolved Hide resolved
Comment on lines 206 to 209
// Event types without data use `true` as a placeholder.
self.push_line(&format!(
"table.insert(polling_payload_queues[{id}], if value == nil then true else value)"
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if true is being used as a placeholder, then what happens to real true values? I recommend we use some "null" userdata to represent nil

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we use a counter for events w/o data instead of an array - sorta like how it works already for queuing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we use a counter for events w/o data instead of an array - sorta like how it works already for queuing

Something needs to be returned to the loop or iteration will end, there's no player to return on the client.

Copy link
Contributor Author

@Ketasaja Ketasaja Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if true is being used as a placeholder, then what happens to real true values? I recommend we use some "null" userdata to represent nil

I don't see much benefit to this unless you assign a metatable that errors on everything, but you don't like metatables. I could make this change though.

for meaningless_value in event do
    if meaningless_value then
        -- Same behavior either way.
    end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd also need to lie to the type system because newproxy(true) returns any, which would be worse than true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to do something because if we use true then boolean? becomes impossible

zap/src/output/luau/client.rs Outdated Show resolved Hide resolved
zap/src/output/luau/client.rs Outdated Show resolved Hide resolved
zap/src/output/luau/server.rs Outdated Show resolved Hide resolved
zap/src/output/luau/server.rs Outdated Show resolved Hide resolved
@Ketasaja
Copy link
Contributor Author

Ketasaja commented Oct 8, 2024

I changed the placeholder value to an explanatory string literal because I don't think a newproxy userdata adds any benefit. newproxy values are any type, which is worse than a singleton. A metatable that errors on everything might be useful, but Zap doesn't like metatables.

for meaningless_value in event do
    if meaningless_value then
        -- Same behavior whether `meaningless_value` is a `newproxy` userdata or not.
    end
end

I can change this if necessary.

@sasial-dev
Copy link
Collaborator

Does it matter that newproxy is typed as any? We can type it as never ourselves if we really want to.

@sasial-dev
Copy link
Collaborator

I'd also prefer we didn't use a string singleton.

@Ketasaja
Copy link
Contributor Author

Ketasaja commented Oct 9, 2024

Does it matter that newproxy is typed as any? We can type it as never ourselves if we really want to.

never won't cause type errors. A string singleton will at least cause type errors in strict mode whenever it's used in a non-string or other string literal context, and would be obvious once inspected at runtime. If the userdata is passed around it won't be obvious what it is outside of networking code when debugging.

If you want the change to userdata with that said, I'll make it, but I think a string singleton is the least wrong option.

@sasial-dev
Copy link
Collaborator

*unknown, not never sorry

@sasial-dev
Copy link
Collaborator

As discussed with @jackdotink - I think this PR is overcomplicated and has a ton of merge conflicts. We'd still be open to a new implimentation, though!

@sasial-dev sasial-dev closed this Nov 27, 2024
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.

3 participants