XAMPPRocky / fluent-templates

Easily add Fluent to your Rust project.
Apache License 2.0
136 stars 28 forks source link

Remove flume default features #36

Closed robjtede closed 2 years ago

robjtede commented 2 years ago

For some annoying reasons that have nothing to do with the quality of this lib, having flume as a dependency is preventing some compilations. This PR removes the default features to make sure nanorand doesn't get included.

The real culprit is that nanorand v0.7.0 doesn't guard its js feature with any cfg, which when used by flume (default features) results in a nasty circular dependency such as:

 › cargo build
    Updating crates.io index
error: cyclic package dependency: package `serde_json v1.0.82` depends on itself. Cycle:
package `serde_json v1.0.82`
    ... which satisfies dependency `serde_json = "^1.0"` of package `wasm-bindgen v0.2.81`
    ... which satisfies dependency `wasm-bindgen = "^0.2.81"` of package `js-sys v0.3.58`
    ... which satisfies dependency `js-sys = "^0.3"` of package `getrandom v0.2.7`
    ... which satisfies dependency `getrandom = "^0.2.3"` of package `ahash v0.7.6`
    ... which satisfies dependency `ahash = "^0.7.0"` of package `hashbrown v0.12.2`
    ... which satisfies dependency `hashbrown = "^0.12"` of package `indexmap v1.9.1`
    ... which satisfies dependency `indexmap = "^1.5.2"` of package `serde_json v1.0.82`
    ... which satisfies dependency `serde_json = "^1.0"` of package `acme-micro v0.12.0`

This seems like the simplest solution for my immediate issue (trying to add a fluent templates example to the Actix Web examples repo) but given that flume is not adding any value where it is used here, it seems prudent to reduce dependencies, too.

XAMPPRocky commented 2 years ago

Thank you for your PR! Right now I’m not sure I’d accept this as is, however I think there’s solution in fluent-templates.

but given that flume is not adding any value where it is used here, it seems prudent to reduce dependencies, too.

This is not true, flume is measurably significantly faster than std’s channel implementation, you will notice it, especially in large directories.

It’s a shame that nanorand doesn’t guard the cfg, which should be fixed, but the read_from_dir code will not work in a WASM environment anyway, so if instead of removing flume, could you just cfg out the filesystem code for WASM environments? That would be acceptable to me. You also then don’t need to change fluent-template-macros, since it always runs on the host.

robjtede commented 2 years ago

Fair enough that you know about the performance impacts. Removing default features also breaks the cycle so opted for that.

robjtede commented 2 years ago

It’s a shame that nanorand doesn’t guard the cfg

It does on it's master branch but sadly that's not released yet.

robjtede commented 2 years ago

Completed the example BTW: https://github.com/actix/examples/tree/master/templating/fluent. Really liking this lib.

robjtede commented 2 years ago

Any thoughts on this solution?

XAMPPRocky commented 2 years ago

Oh sorry, I thought I had already merged this. Thank you for your PR, and congrats on your first contribution! 🎉