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

Limit pub exposure in 2.x #10158

Merged
merged 15 commits into from
Jul 31, 2024
Merged

Limit pub exposure in 2.x #10158

merged 15 commits into from
Jul 31, 2024

Conversation

chippers
Copy link
Member

@chippers chippers commented Jul 1, 2024

[in progress]

  • Asset
    • pub fn tauri::AssetResolver<R>::get(&self, path: alloc::string::String) -> core::option::Option<tauri::Asset>
    • only used as a return - not likely to be constructed because it's not passed as a parameter to any tauri api. non-exhaustive seems the least invasive
  • CallbackFn
    • hard to make this not pub because either way it's public on the TS/JS side and is serialized. only leftover question is if u32 is enough since there's a decent chance of collision at only a few to a dozen thousand. not a common use case, but could see something like passing a ton of request to Tauri core to be executed by the async runtime and waiting for them all to resolve. this would still be problematic to increase due to js having an effective safe integer limit of 53 bits - so we would need to use a 53 bit type
    • iOS
      • iOS has its own one with pub stuff to communicate with swift, again ffi issues with changing the signature if they are not always in lockstep (i.e. the code defining the struct always generates the ffi bindings before building - we don't currently do this)
  • CommandItem
    • typically created by our own macro code, don't see a need for this to change. Can be used by others if they create a 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 happen
  • Invoke
    • used and created by our own macro code, don't see a need for this to change. marked as #[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
    • fine - simple newtype wrapper around a serde_json::Value
  • InvokeRequest
    • pub fn tauri::webview::Webview<R>::on_message(self, request: tauri::webview::InvokeRequest, responder: alloc::boxed::Box<tauri::ipc::OwnedInvokeResponder<R>>)
      pub fn tauri::WebviewWindow<R>::on_message(self, request: tauri::webview::InvokeRequest, responder: alloc::boxed::Box<tauri::ipc::OwnedInvokeResponder<R>>)
    • while this is also an ipc message, it is a struct. marking it #[non_exhaustive] lets us add future Option 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 constructor
    • upside: constructing it from the rust side doesn't make too much sense because it contains the callback and error fn, meaning they are trying to use our invoke type in their own custom invoke system. (which they can't anyways because of the invoke_key)
    • it is exposed in public api thru WebView::on_message, but is thru a parameter meaning even if they are using it, they are not creating it.
    • marked as #[non_exhaustive] without adding a constructor because it doesn't really make sense to need one. all creating happens in our crate
  • WebviewManager
    • because of the recent addition of non-pub 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?
    • if so we need to create constructors
  • ProgressBarState
    • seems simple enough and POD. perhaps only thing is status as we cannot add new statuses because it's not #[non_exhaustive]. @amrbashir do you see a need for the status enum to change in v2?

@chippers chippers self-assigned this Jul 1, 2024
@chippers
Copy link
Member Author

chippers commented Jul 1, 2024

We expose the InvokeRequest in the test module, should we unmark this non-exhaustive and instead make it an unstable internal API - or just provide a constructor for it? It only makes sense to construct internally and to mock it for tests

@amrbashir
Copy link
Member

ProgressBarState

  • seems simple enough and POD. perhaps only thing is status as we cannot add new statuses because it's not #[non_exhaustive]. @amrbashir do you see a need for the status enum to change in v2?

in v2, probably not, and will be rarely updated in later versions tbh but let's mark it as #[non_exhaustive] just to be safe

@lucasfernog-crabnebula
Copy link
Contributor

InvokeRequest construction is required to test IPC calls (tauri::test::assert_ipc_response and tauri::test::get_ipc_response)

@lucasfernog-crabnebula
Copy link
Contributor

WebviewManager do not need to be created by externals

@tweidinger
Copy link
Contributor

InvokeRequest construction is required to test IPC calls (tauri::test::assert_ipc_response and tauri::test::get_ipc_response)

IMHO this is also required for fuzzing, so it should stay exposed but we could mark it as unstable.

@chippers
Copy link
Member Author

Still need to work on types coming from tauri-utils and tauri-runtime that end up getting exposed through tauri APIs and callbacks.

@vdang-crabnebula
Copy link
Contributor

Hey!
The fuzzer is currently "emulating" the JS side to call the Tauri backend, so there are entities from this list that I'd prefer having behind a feature gate rather than blocking them.

  • pub fn tauri::WebviewWindow<R>::on_message(self, request: tauri::webview::InvokeRequest, responder: alloc::boxed::Box<tauri::ipc::OwnedInvokeResponder<R>>) is used to call the Tauri backend
  • I currently create the InvokeRequest manually hence I also use CallbackFn and InvokeBody

My take is to mark InvokeRequest as #[non-exhaustive] but provide a constructor marked unstable or feature-gated.
Also on_message could be unstable or feature-gated too.

@chippers
Copy link
Member Author

chippers commented Jul 23, 2024

We can use serde's flatten attribute for serde de/serializable stuff (especially configs) to allow future config values to exist.

e.g. for LinuxConfig

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 ExtraConfig for the configs that basically wraps the hashmap so that it doesnt look like there is a random hashmap for configs.

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.

@chippers
Copy link
Member Author

chippers commented Jul 25, 2024

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

  • need to add the extend field and the 2 attributes to most configs (and probably the acl also)
  • since it doesn't get parsed to the type, care has to be taken for complex types to prevent too much duplicate work for pulling a config out (e.g. you should cache the config value before a loop)
  • cant implement Eq (probably shouldn't be a problem)

Upsides:

  • Dont need to create a builder for every single config, along with getters and setters
  • Only need to create the extended struct config for the ones we actually need to extend

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.

Copy link
Contributor

github-actions bot commented Jul 25, 2024

Package Changes Through 804c502

There are 4 changes which include tauri-cli with prerelease, @tauri-apps/cli with prerelease, tauri-utils with prerelease, tauri with prerelease

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
tauri-utils 2.0.0-beta.19 2.0.0-beta.20
tauri-bundler 2.0.1-beta.19 2.0.1-beta.20
tauri-runtime 2.0.0-beta.21 2.0.0-beta.22
tauri-runtime-wry 2.0.0-beta.21 2.0.0-beta.22
tauri-codegen 2.0.0-beta.19 2.0.0-beta.20
tauri-macros 2.0.0-beta.19 2.0.0-beta.20
tauri-plugin 2.0.0-beta.19 2.0.0-beta.20
tauri-build 2.0.0-beta.19 2.0.0-beta.20
tauri 2.0.0-beta.25 2.0.0-beta.26
@tauri-apps/cli 2.0.0-beta.23 2.0.0-beta.24
tauri-cli 2.0.0-beta.23 2.0.0-beta.24

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@lucasfernog
Copy link
Member

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

  • need to add the extend field and the 2 attributes to most configs (and probably the acl also)
  • since it doesn't get parsed to the type, care has to be taken for complex types to prevent too much duplicate work for pulling a config out (e.g. you should cache the config value before a loop)
  • cant implement Eq (probably shouldn't be a problem)

Upsides:

  • Dont need to create a builder for every single config, along with getters and setters
  • Only need to create the extended struct config for the ones we actually need to extend

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.

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

@lucasfernog
Copy link
Member

and perhaps document that people should use ..Default::default()

@chippers
Copy link
Member Author

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.

@chippers chippers marked this pull request as ready for review July 30, 2024 08:10
@chippers chippers requested a review from a team as a code owner July 30, 2024 08:10
@tweidinger tweidinger requested a review from lucasfernog July 30, 2024 08:16
Copy link
Member

@lucasfernog lucasfernog left a 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

@lucasfernog lucasfernog merged commit eaec5fe into dev Jul 31, 2024
28 checks passed
@lucasfernog lucasfernog deleted the feat/2.x-pub-exposure branch July 31, 2024 13:54
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.

6 participants