search

DioxusLabs / dioxus

Fullstack GUI library for web, desktop, mobile, and more.
https://dioxuslabs.com
Apache License 2.0
18.5k stars 703 forks source link

add file size to FileEngine #2323

Open rogusdev opened 2 weeks ago

rogusdev commented 2 weeks ago

This allows file sizes to be checked before loading the entire file into memory (e.g. with read_file) when it might be too big. (And without requesting the native file to do this check manually.)

rogusdev commented 2 weeks ago

Note: there were no automated tests for any of the affected structs that I could see, so I simply tested with:

cd packages/web && cargo build && cd -
cd packages/html && cargo build -F native-bind && cd -

Which seemed to catch any compiler errors in the affected code, at least. And, ofc, it works in my own application depending on the new feature :) (manually tested web only tho)

rogusdev commented 2 weeks ago

I don't know how I affected the failing workflow test.

As for the labels: this is a new function that should be indiscernible to any users who don't reach for it. How is it "breaking"? Which is to say, how does this project define "breaking" changes?

Thanks for starting to review it!

jkelleyrtp commented 2 weeks ago

We have a flakey windows test that needs to be resolved - it's not a blocker for your PR!

We define breaking changes as changes that are not semver compatible and I believe adding a new non-default method to a trait is breaking.

https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default

If the trait provided a default (IE read the file and then measure it) then this could get merged as part of 0.5 release cycle. We haven't started doing it yet, but our flow involves keeping a stable branch (0.5) and a nightly branch (main) and then backporting fixes onto the stable branch. However we're currently still in the release phase where everything is fixes and all the new features are being held for a bit.

rogusdev commented 2 weeks ago

I'm in no rush personally -- I'm using my local version now.

I suspect that part of semver is referring to libraries where others are implementing that trait. Certainly then it would be a breaking changes, but that seems out of place in the context of dioxus. That said, I defer to your process -- and certainly bug fixes are more important than this right now!

Thanks :)

ealmloff commented 2 weeks ago

Flakey windows tests are fixed in https://github.com/DioxusLabs/dioxus/pull/2332 🙂

rogusdev commented 2 weeks ago

Flakey windows tests are fixed in #2332 🙂

I rebased and force pushed with those changes so you can re-run to get everything passing.