Skip to content

Move to toml from toml_edit #512

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

Merged
merged 8 commits into from
Jul 10, 2025
Merged

Move to toml from toml_edit #512

merged 8 commits into from
Jul 10, 2025

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jul 9, 2025

This was originally a normal dependency bump for our lockfiles, but...

toml v0.9.0 has released with fun performance improvements! Check out Ed Page's blog post on it, there's some cool insights there.

One of the biggest things that stood out to me is that toml no longer depends on toml_edit. My original reasoning for using toml_edit instead of the usual serde + toml combo is because toml v0.8 and earlier used toml_edit under the hood. Since they both used the same infrastructure, and we didn't really need serde, I figured it would be more efficient to just use toml_edit.

This is no longer the case! toml now does its own parsing and is faster than toml_edit. We don't need the extra functionality of toml_edit, so this PR drops it completely and switches to toml + serde.

Most of the migration was simple and 1:1, but there are some spots that need extra testing to ensure they stay the same behaviorally:

  • impl Display for CliConfig (it may print CliConfig slightly differently)
  • configure_default_web_profiles() (I believe DocumentMut pretty much translates Table, but it needs confirmation!)

BD103 added 5 commits July 8, 2025 21:00
The implementation is just as simple, `toml_edit` preserves extra information that we don't need, and `toml`'s v0.9.0 release provides some nice speedups.
@BD103 BD103 added C-Dependencies A change related to dependencies A-Cross-Cutting Affects multiple areas of the repository D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review The PR needs to be reviewed before it can be merged labels Jul 9, 2025
Copy link
Collaborator

@DaAlbrecht DaAlbrecht left a comment

Choose a reason for hiding this comment

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

new:

❯ bevy build web -v
debug: using defaults from bevy_cli config:
      features = ["dev"]
      default-features = false
      rustflags = [
          "--cfg",
          'getrandom_backend="wasm_js"',
      ]
info: compiling to WebAssembly...

old:

❯ bevy build web -v
debug: using defaults from bevy_cli config:
      features = ["dev"]
      default-features = false
      rustflags = ["--cfg", 'getrandom_backend="wasm_js"']

I think that's absolutely fine, but what annoys me a bit now that I see it (was already like this before):
image

@BD103
Copy link
Member Author

BD103 commented Jul 10, 2025

I think that's absolutely fine, but what annoys me a bit now that I see it (was already like this before):

Hmmm now that's bothering me too! Let me make a little change real quick...

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review The PR needs to be reviewed before it can be merged labels Jul 10, 2025
@BD103
Copy link
Member Author

BD103 commented Jul 10, 2025

Much better!

image

@BD103 BD103 removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jul 10, 2025
@BD103 BD103 enabled auto-merge (squash) July 10, 2025 11:36
@BD103 BD103 merged commit 2425485 into main Jul 10, 2025
10 checks passed
@BD103 BD103 deleted the update-deps branch July 10, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Affects multiple areas of the repository C-Dependencies A change related to dependencies D-Straightforward Simple bug fixes and API improvements, docs, test and examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants