leptos-rs / cargo-leptos

Build tool for Leptos (Rust)
MIT License
314 stars 87 forks source link

Maintenance patch. #274

Open martinfrances107 opened 3 weeks ago

martinfrances107 commented 3 weeks ago

Ran the standard battery of checks and fixed up deprectaed issues, clippy warnings etc.

cargo update
cargo outdated
cargo fmt
cargo clippy

Lots of packges updated. In particular lightningcss moved from 47-55(alpha) This is a essential first step in resolving outstanding lightningcss issues.

Hightlights :-

PS Notify is 2 major number behind .. I am going to create a separate PR with Notifty only changes.

martinfrances107 commented 3 weeks ago

Just making notes while I figure this out....

Here are two errors related to the window build I dont have a window machine, so I can't recreate the issue locally - So I am going to have to think my way through the problem

error[E0432]: unresolved import `winapi::shared::winerror`
  --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\notify-4.0.17\src\windows.rs:11:21
   |
11 | use winapi::shared::winerror::ERROR_OPERATION_ABORTED;
   |                     ^^^^^^^^ could not find `winerror` in `shared`

   Compiling reqwest v0.12.4
error[E0308]: mismatched types
   --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\notify-4.0.17\src\windows.rs:276:29
    |
276 |         overlapped.hEvent = request_p;
    |         -----------------   ^^^^^^^^^ expected `winapi::ctypes::c_void`, found `libc::c_void`
    |         |
    |         expected due to the type of this binding
    |
    = note: `libc::c_void` and `winapi::ctypes::c_void` have similar names, but are actually distinct types
note: `libc::c_void` is defined in crate `core`
   --> /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04\library\core\src\ffi\mod.rs:173:1
note: `winapi::ctypes::c_void` is defined in crate `winapi`
   --> C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\winapi-0.3.9\src\lib.rs:38:5
    |
38  |     pub enum c_void {}
    |     ^^^^^^^^^^^^^^^

Reverse engineering the break

Change to "brotli" triggers changes to "flexilogger" Changes to "flexilogger" triggers changes to "chrono" Change to "chrono" triggers changes to "windows_targets"

So I think reverting to the change to brotli would fix the issue But I want somthing cleaner.

martinfrances107 commented 2 weeks ago

The last commit is experimental .. more work is required.

shivjm commented 2 weeks ago

I’m on Windows 10 and I can confirm the winapi build issue. Installing martinfrances107@98ca5bed works, but it looks like watching files isn’t functional, or at least isn’t functioning correctly, because I see a lot of these events without any sort of rebuilding (unless I restart the command, of course):

       Event { kind: Modify(Any), paths: ["C:\\Media\\src\\my\\hledger-web-rs\\src\\app.rs"], attr:tracker: None, attr:flag: None, attr:info: None, attr:source: None }
martinfrances107 commented 1 week ago

Updated version of notify is a major change.

In this new regime "Rename" events can supply a variable number of files.

Now notify is cross platform and has lots of new fine grained events.

I am not sure I have preserved the intent of the original.. I am still looking at this.

shivjm commented 5 days ago

Just chiming in once more to say that cargo leptos watch still isn’t responding to file changes for me with martinfrances107@558bef52. My apologies if this is out of scope; I only started using Leptos with this branch, so I don’t know whether it’s changed behaviour or not.

martinfrances107 commented 4 days ago

I will have another look, this issue started as a maintence patch.

I tried to do the changes associated with the notfiy crate in an isolated PR as it was a project in itself.

I am a big fan of keeping PRs small and isolated to one issue... BUT... here the slowly increasing complexity is justified.

regarding the changes in notify.rs try_new() -- it lacks associated tests.

So NEXT .. I am going to add the smallest test to trigger on change of file contents.

martinfrances107 commented 2 days ago

The next PR is a outline of the test

// Overivew :-
//
// SETUP: create a file in a valid project.
//
// 1) Construct watching mechanism.
//
// 2) Modfify the file.
//
// 3) Assert the mechanism observed a valid event.
//
// TEARDOWN: delete the file.
#[ignore]
#[test]
fn change_file_contents() {
  /// ....
}

I am currently "#[ignore]ing" the new test - its not passing, I am in the middle of debugging it.

But I see enough detail to cause an architectural discussion in the background.

from the commit log -

"A structural issue, if no event is generated then the test hangs."

I am going the have to insert a timeout mechansim that says after N ms -- The event is declared overdue and a fail issued.

Making test dependant on a minimim execution time is borderline bad practise ... We are making assumption about the resouces given to the testing infrasture..

But in this case I think an exception has to be made?