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

do not force default features on consumers #8

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Niedzwiedzw
Copy link

No description provided.

@matthunz
Copy link
Member

I would love to see cross platform support like this!

Unfortunately this crate still relies on a few dependencies that are still web-only, like dioxus-resize-observer. Using the new JS eval techniques it should be possible to migrate to native platforms

@marc2332 marc2332 added the enhancement New feature or request label Feb 13, 2025
@Niedzwiedzw
Copy link
Author

Niedzwiedzw commented Feb 13, 2025

the problem is that adding this crate to dependencies enables the dioxus logging functionality by default, and I'm rolling my own logger which then panics.

@marc2332
Copy link
Member

Just wondering, what specific features of dx are enabled by default that could not be wanted when using this library? It doesn't make sense, the features you disabled are enabled anyway because of the other dx dependencies used here, so it doesn't really change anything

Few things to improve nonetheless:

  • Ignore the lock in git
  • use dioxus-lib instead of dioxus

@marc2332
Copy link
Member

the problem is that adding this crate to dependencies enables the dioxus logging functionality by default, and I'm rolling my own logger which then panics.

Fair point, but I'd just switch to dioxus-lib then

@marc2332 marc2332 self-requested a review February 13, 2025 17:29
@Niedzwiedzw
Copy link
Author

the problem is that adding this crate to dependencies enables the dioxus logging functionality by default, and I'm rolling my own logger which then panics.

Fair point, but I'd just switch to dioxus-lib then

all right let me try...

@Niedzwiedzw
Copy link
Author

mkay I'm down to 1 error

error[E0599]: no method named `as_web_event` found for reference `&MountedData` in the current scope
  --> src/use_list.rs:86:32
   |
86 |             let elem = mounted.as_web_event();
   |                                ^^^^^^^^^^^^ method not found in `&MountedData`

For more information about this error, try `rustc --explain E0599`.

@Niedzwiedzw
Copy link
Author

@marc2332
Copy link
Member

it works now?

@Niedzwiedzw
Copy link
Author

still testing

@marc2332
Copy link
Member

Moving dioxus behind a feature won't fix it, we probably also need to change https://github.com/dioxus-community/dioxus-use-mounted/blob/779bda19f6c87125170f8859701ba1df172813ff/Cargo.toml#L10 to dioxus-lib, if you can setup a PR really quick I can merge it and release it

@Niedzwiedzw
Copy link
Author

ok it's still getting pulled by dioxus-use-mounted - it's very small though, I'll try patching it there as well

@Niedzwiedzw
Copy link
Author

so once this and dioxus-community/dioxus-use-mounted#6 get merged it should work

@marc2332
Copy link
Member

left some comments

@marc2332
Copy link
Member

@Niedzwiedzw
Copy link
Author

@Niedzwiedzw
Copy link
Author

nothing needs to be done here cause the specified version is 0.3 right?

@marc2332
Copy link
Member

nothing needs to be done here cause the specified version is 0.3 right?

Yeah, just make sure to run cargo update so it fetches the latest release

@Niedzwiedzw
Copy link
Author

all right it officially works on my machine™

@marc2332
Copy link
Member

Nice!

@@ -49,4 +49,3 @@ fn app() -> Element {
onmounted: move |event| list.mounted.onmounted(event)
})
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this was removed? seems like an accident


[dev-dependencies]
console_error_panic_hook = "0.1.7"
console_error_panic_hook = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console_error_panic_hook = "0.1"
console_error_panic_hook = "0.1"
dioxus = { version = "0.6", features = ["web"] }

use dioxus_lazy::{lazy, List};
use dioxus_lib::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use dioxus_lib::prelude::*;
use dioxus::prelude::*;

@@ -18,5 +18,5 @@ fn main() {
dioxus_logger::init(Level::INFO).unwrap();
console_error_panic_hook::set_once();

dioxus::launch(app);
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
launch(app)

use dioxus_lazy::{lazy, List};
use dioxus_lib::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use dioxus_lib::prelude::*;
use dioxus::prelude::*;

@@ -17,6 +17,5 @@ fn app() -> Element {
fn main() {
dioxus_logger::init(Level::INFO).unwrap();
console_error_panic_hook::set_once();

dioxus::launch(app);
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
launch(app)

use dioxus_lazy::{lazy, Direction, UseList};
use dioxus_lib::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use dioxus_lib::prelude::*;
use dioxus::prelude::*;

@@ -16,6 +16,5 @@ fn app() -> Element {
fn main() {
dioxus_logger::init(Level::INFO).unwrap();
console_error_panic_hook::set_once();

dioxus::launch(app);
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dioxus_web::launch::launch_cfg(app, dioxus_web::Config::new())
launch(app)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants