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

refactor(core): remove Params and replace with strings #2191

Merged
merged 14 commits into from Jul 15, 2021

Conversation

chippers
Copy link
Member

@chippers chippers commented Jul 9, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Docs
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • YES

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
See #2179 for prior work that sparked this PR. I've added most of my TODO notes to the change log instead of throughout the code because the diff is large enough to lose track of them easily.

The conversation in discord spanned a couple hours and a few more smaller discussion after that, so I will try to sum up the motivation behind the breaking change in bullet points:

  • Params was added during a large refactor multiple months ago, and allowed us to put user-defined types throughout the codebase (bonus: statically dispatched).
  • Features grew and the list of associated types grew. We used Args as a private markerParams type to simplify the generic parameters in most of the internal code but it needed a lot of generics on it. This was of course particularly painful where we had to use Args instead of Params such as the builder and internal window manager.
  • Not many people were taking much advantage of the strong types possible through Params (ofc it was just released during beta) and instead struggled to comprehend the now mentally complex Rust API (particularly to people learning or new to Rust) in places. The not many people measurement is vague, but based on hardly any questions in the discord were about using it and instead about trying to use strings in the API where Params made things more complex type-wise. Also in a cursory GitHub search of projects using Tauri beta the first 4-5 pages had no projects where non-default (String) types were used.
  • Because of the nature of the traits used in the associated types on Params, the types effectively always had to represent a string in some form (Display or Serialize). Storing those types directly with static dispatch in internal implementation is not really necessary as they had to be turned into a string at some point anyways.
  • Due to the previous point, a strongly typed API for tauri doesn't require being injected into the internals for all current cases. It can instead live in another crate (TBD) with a less stable API as we figure out the most ergonomic way to allow complex compile time type checking without ruining the ease of development without it.

So this PR is the result of those bullet points. Because another crate can effectively wrap tauri with a stronger API, the complexity introduced in tauri itself for it is unnecessary and should be shed before v1. The more strongly typed API can be created separately at a later time.

This is a breaking change. It completely removes Params and replaces all the associated types previously on it with either strings (Event, Label, MenuId, SystemTrayMenuId) or trait objects (Assets). Items that previously took Params now take only Runtime. It also removes traits that served solely to help manage those string types, such as Tag, TagRef, and MenuId.

I was (hopefully) consistent about putting impl Into<String> in places where we are accepting an owned string in a public api, let me know if I missed any. We might also want to change all the public API that accepts &str to impl AsRef<str> but I didn't do that as I went.

We should really flesh out the changelog on this change since this is a decently sized breaking change.

Marked as draft until we finish off the todos and the change file.

@wusyong
Copy link
Member

wusyong commented Jul 10, 2021

Thanks! I think this will really improve a lot on code readability and still keep the runtime generic configurable.

core/tauri/Cargo.toml Outdated Show resolved Hide resolved
.changes/weak-typing.md Outdated Show resolved Hide resolved
@chippers chippers self-assigned this Jul 14, 2021
@chippers chippers marked this pull request as ready for review July 14, 2021 02:29
@chippers chippers requested a review from a team as a code owner July 14, 2021 02:29
@chippers chippers requested review from a team July 14, 2021 02:29
@wusyong wusyong merged commit fd8fab5 into dev Jul 15, 2021
@wusyong wusyong deleted the refactor/weak-typing branch July 15, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants