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

Move the LiveSocket and LiveChannel api into one central object. #289

Merged
merged 57 commits into from
Feb 5, 2025

Conversation

mobile-bungalow
Copy link
Contributor

@mobile-bungalow mobile-bungalow commented Jan 3, 2025

This is not ready for review yet. goals are as follows.

  1. Move LiveSocket and LiveChannel api's into a single LiveViewClient object.
    • Create message sending method that can intercept diffs and errors.
    • Rewrite integration tests and examples to use the LiveViewClient
    • Encapsulate the live reload channel, force reload on assets_change
    • handle patch, asset reload, live redirect, events.
  2. Toggleable logging, stack traces.
    • Use oslog, android log, etc. for specific platforms
  3. Remove old API
    • Trial implementation in live view swift ui

Copy link
Contributor

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I know this is just a draft. It looks pretty good. I had a few minor nits.

@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Maybe this was added by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah eventually I want to parse style sheets and send them over FFI. But this is far out.


// Each test runs in a separate thread and should make requests
// as if it is an isolated session.
#[cfg(test)]
thread_local! {
static TEST_COOKIE_JAR: Arc<Jar> = Arc::default();
pub static TEST_COOKIE_JAR: Arc<Jar> = Arc::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be pub (crate).

Copy link
Contributor Author

@mobile-bungalow mobile-bungalow Jan 8, 2025

Choose a reason for hiding this comment

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

The plan for this PR

  • get a working centralized version of the API while keeping the old LiveSocket one.
  • run both side by side in as many of the tests as possible to ensure parity - then delete LiveSocket

the cookie jar will be completely gone by the end of it

@@ -37,6 +43,7 @@ browser = [
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
serde_urlencoded = "0.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a optional dependency that's enabled with the liveview-channels feature.

Comment on lines 22 to 52
pub fn init_log(level: LogLevel) {
INIT_LOG.call_once(|| {
let env = Env::default();
let mut builder = Builder::from_env(env);
builder
.format(|buf, record| {
if record.level() == log::Level::Error {
writeln!(
buf,
"[{}] {} {}:{} - {}",
record.level(),
record.target(),
record.file().unwrap_or("unknown"),
record.line().unwrap_or(0),
record.args()
)
} else {
writeln!(
buf,
"[{}] {} - {}",
record.level(),
record.target(),
record.args()
)
}
})
.filter(None, level.into())
.try_init()
.expect("LOG INITIALIZATION FAILED");
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Does env logger work on the various platforms? Like, debugging on device kinda requires using oslog. I feel like there was an android one (that I never got working).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a stub for now. I am going make this module way more configurable, but for testing purposes env log is fine. in the end it will be a multi-log with an env log and platform specific log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other goal here - each platform will want something akin to a developer console. Providing a callback interface for appending to an application developer debug log which can filter out core events, present warnings and diagnostics, is an goal.

Copy link
Contributor

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I did some more random reviews but got distracted with other things.

session_data: &Mutex<SessionData>,
redirect: Option<String>,
) -> Result<Arc<LiveChannel>, LiveSocketError> {
let ws_timeout = std::time::Duration::from_millis(config.websocket_timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really matter but I'm a fan of use std::time::Duration and such at the top of the file and then Duration::from_millis here and below in join_livereload_channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is fair

};

let mut buffer = Vec::new();
let store_guard = self.store.lock().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Given that you call this from an impl Drop, you might not wanna have an unwrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is fair, I'm going to move this out of drop. I thought I was being clever by making this run on clean up - but we pretty much have to run it after every insertion just in case something goes wrong anyways.

@mobile-bungalow mobile-bungalow merged commit 6abc4bf into liveview-native:main Feb 5, 2025
19 checks passed
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.

2 participants