-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Limit pub exposure in 2.x #10158
Limit pub exposure in 2.x #10158
Conversation
We expose the |
in v2, probably not, and will be rarely updated in later versions tbh but let's mark it as |
InvokeRequest construction is required to test IPC calls ( |
WebviewManager do not need to be created by externals |
IMHO this is also required for fuzzing, so it should stay exposed but we could mark it as unstable. |
Still need to work on types coming from |
Hey!
My take is to mark |
We can use serde's flatten attribute for serde de/serializable stuff (especially configs) to allow future config values to exist. e.g. for add #[serde(flatten)]
pub extra: HashMap<String, serde_json::Value>, to the public fields. (note: we would use something more like the acl's value so it works with json/toml) Then provide helpers for using new stuff. Let's say we add flatpak support to this config. impl LinuxConfig {
/// The flatpak config for v2.1+
pub fn flatpak(&self) -> Option<&FlatpakConfig> {
self.extra.get("flatpak")
}
/// Sets the flatpak config for v2.1+
pub fn set_flatpak(&mut self, config: FlatpakConfig) {
self.extra.insert("flatpak".into(), config);
}
} We can make a small wrapper called This allows the docs to show that a specific config has a new field while still letting the configs stay all pub for easy construction, access, and editing - which is useful because they are mostly PODs. |
somthing like this /// A way to extend a `pub` config object without a breaking change.
///
/// Implementing a config like the way in the example hides the extensibility from the Schema,
/// keeps the Schema without additional properties, and transparently adds fields from an
/// extended config to the original Schema - completely hiding the code (and semver breaking change
/// benefits) from using the config with the Schema.
///
/// To use it, add a field to the config object you may want to extend in the future.
/// When the time comes to extend it, you can point the schema attribute to your
/// new extended config object, and then expose the new items with methods on the original.
///
/// # Example
///
/// An extendable config object before extending.
///
/// ```rust
/// # use schemars::JsonSchema;
/// # use serde::{Deserialize, Serialize};
/// # use tauri_utils::config::ExtendConfig;
///
/// #[derive(Debug, Serialize, Deserialize, JsonSchema)]
/// struct MyConfig {
/// pub name: String,
///
/// #[schemars(with = "()")]
/// #[serde(flatten)]
/// pub extend: ExtendConfig
/// }
/// ```
///
/// Afterwords, you realize they sometimes also need to specify their age.
///
/// ```rust
/// # use schemars::JsonSchema;
/// # use serde::{Deserialize, Serialize};
/// # use tauri_utils::{acl::{Number, Value}, config::ExtendConfig};
///
/// #[derive(Debug, Default, Serialize, Deserialize, JsonSchema)]
/// #[non_exhaustive]
/// struct MyConfigExtended {
/// pub age: Option<u8>
/// }
///
/// #[derive(Debug, Default, Serialize, Deserialize, JsonSchema)]
/// struct MyConfig {
/// pub name: String,
///
/// #[schemars(with = "MyConfigExtended")]
/// #[serde(flatten)]
/// pub extend: ExtendConfig
/// }
///
/// impl MyConfig {
/// /// Grab the age from the config.
/// pub fn age(&self) -> Option<u8> {
/// self.extend.get("age").and_then(|value| match value {
/// Value::Number(Number::Int(i)) => i.try_into().ok(),
/// Value::Number(Number::Float(f)) => (*f as i64).try_into().ok(),
/// _ => None
/// })
/// }
/// }
/// ```
#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize, JsonSchema)]
#[serde(transparent)]
pub struct ExtendConfig {
inner: BTreeMap<String, Value>,
}
impl ExtendConfig {
/// Get the `Value` of the key if it exists.
pub fn get(&self, key: &str) -> Option<&Value> {
self.inner.get(key)
}
/// Set the key to a new `Value`, replace the old one if it exists.
pub fn set(&mut self, key: String, value: Value) -> Option<Value> {
self.inner.insert(key, value)
}
} Downsides
Upsides:
both options allow the schema to be represented correctly without additional properties being allowed (so using it to write a config is still just as smooth) cc @lucasfernog im not sure which is more desirable here. This extension leaves things open for us in the future and we only need to extend the things we need to extend - just with some upfront fields that may never get used. The builders with getters/setters would be more typical but annoying to construct by hand. However, since we mostly de/serialize the config values, manual construction mostly happens in macros which is easy to do. This would still have us almost double our config struct count since we would need builders for almost all of them if we want them to be extendable in the future without a breaking change. |
Package Changes Through 804c502There are 4 changes which include tauri-cli with prerelease, @tauri-apps/cli with prerelease, tauri-utils with prerelease, tauri with prerelease Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
can't we just keep marking the config module as unstable? it should only be manually constructed by us, but we can't use #[non_exhaustive] because of our code generation. I feel like this change while semver compatible and correct would make future improvements even more complicated - we already have to go through a lot with CLI, runtime traits, wry implementation etc |
and perhaps document that people should use ..Default::default() |
I didn't find any callbacks compelling enough to create a new api for, most of them include a tauri-defined item so we can always add onto there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the change file manually in the RC PR
[in progress]
Asset
CallbackFn
CommandItem
CommandArgs
for their own type, but they won't create this type so expanding it should be ok. However, we may WANT this to be a breaking change so the advanced user is notified of a new field that we deemed important enough to add. We can mark this like we mark the other internal command stuff, as explicitly not treated as stable and breaking changes can happenInvoke
#[doc(hidden)]
to discourage using it directly (there isn't a need unless you are creating your own custom command handlers and wrappers - not something we support)InvokeError
serde_json::Value
InvokeRequest
#[non_exhaustive]
lets us add futureOption
fields without breaking changes - or even non-option if the core script that sends the invoke request is updated to include the new field automatically - although harder to generate from the Rust side unless with a new constructorWebView::on_message
, but is thru a parameter meaning even if they are using it, they are not creating it.#[non_exhaustive]
without adding a constructor because it doesn't really make sense to need one. all creating happens in our crateWebviewManager
invoke_key
, this struct is no longer constructable outside the crate. fine for this purposes - but is it intended to be buildable from external sources?ProgressBarState
#[non_exhaustive]
. @amrbashir do you see a need for the status enum to change in v2?