Skip to content

Remove vello reexport#5

Closed
DasLixou wants to merge 1 commit intolinebender:mainfrom
DasLixou:no-vello-reexport
Closed

Remove vello reexport#5
DasLixou wants to merge 1 commit intolinebender:mainfrom
DasLixou:no-vello-reexport

Conversation

@DasLixou
Copy link
Contributor

@DasLixou DasLixou commented Apr 7, 2024

I removed the pub use vello; reexport from vello_svg as I found it disturbing when using it on a project that already used vello and ships vello_svg as an addition behind a feature flag, as well as velato, which also reexports vello currently.

@xStrom
Copy link
Member

xStrom commented Apr 7, 2024

Could you go into more detail what you mean by disturbing? Was there a technical difficulty or is this a description of your state of mind?

@DasLixou
Copy link
Contributor Author

DasLixou commented Apr 7, 2024

It is confusing when importing what vello to use

@nuzzles
Copy link
Member

nuzzles commented Apr 7, 2024

It is confusing when importing what vello to use

This isn't a compelling reason, imo.

The onus is on you to understand which version of vello you can use with vello_svg.

In fact, vello is re-exported to help alleviate that confusion. For example, if your use case is only to render an SVG, you would only need vello_svg. Without this, you'd need to keep two dependencies (vello and vello_svg) in sync.

I would however be open to a "vello compatibility chart" on the README, like many crates do with bevy. We currently have a badge in the README, but I don't see that being future proof.

@xStrom
Copy link
Member

xStrom commented Apr 8, 2024

It is confusing when importing what vello to use

That is valuable feedback and we can act on it. Given that the re-export itself is valid, our approach should be something other than removing it. Improving our documentation would be a good step.

In src/lib.rs at the top we already have the following:

This crate also re-exports [`usvg`], to make handling dependency versions easier

We could update it to something like:

We re-export [`vello`] and [`usvg`] for your convenience,
so you can easily use the specific versions that are compatible with Vello SVG.

@nuzzles
Copy link
Member

nuzzles commented May 14, 2024

If I was to weigh in my 2cents, perhaps the best design pattern would be a cargo feature, like we've done with wgpu in vello. I'm just now starting to see that pattern.

We could re-export usvg, conditionally, as a feature of vello_svg. But then I wonder - should it be a default feature?

@nuzzles nuzzles mentioned this pull request Jul 2, 2024
@nuzzles
Copy link
Member

nuzzles commented Jul 2, 2024

I'm going to close this as won't do, because the lib.rs documentation has been updated in #18 to reflect why we re-export these libraries to users, using Kaur's verbage.

@nuzzles nuzzles closed this Jul 2, 2024
@nuzzles nuzzles added the wontfix This will not be worked on label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants