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 file size to FileEngine #2323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rogusdev
Copy link
Sponsor Contributor

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
Copy link
Sponsor Contributor Author

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)

@ealmloff ealmloff added enhancement New feature or request breaking This is a breaking change html Related to the html crate labels Apr 17, 2024
@rogusdev
Copy link
Sponsor Contributor Author

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
Copy link
Member

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
Copy link
Sponsor Contributor Author

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
Copy link
Member

Flakey windows tests are fixed in #2332 🙂

@rogusdev
Copy link
Sponsor Contributor Author

Flakey windows tests are fixed in #2332 🙂

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

@ealmloff ealmloff added breaking This is a breaking change and removed breaking This is a breaking change labels May 14, 2024
Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

I completely agree an API like this needs to exist, but I'm not sure this is what the API should look like long term.

There is some overlap between a file size API, and a file streaming API (#1333). Once file streaming is added, I think it would be better to include everything in a single file object that supports reading the size, reading the stream, name, etc.

// Getting a file object should be very cheap
for file in file_event.files() {
    println!("name: {}", file.name());
    println!("size: {}", file.size());
    println!("contents: {}", file.read().await);
}

That said, a nicer object based file API would require other breaking changes to the current API, so if this were non-breaking it could be fine for 0.5

@rogusdev
Copy link
Sponsor Contributor Author

For sure longer term this should get incorporated into something better, definitely agree. I think including this sooner rather than later allows the need for those bigger changes to be clearer, as well as what they should look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change enhancement New feature or request html Related to the html crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants