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(store)!: more reworks #1860

Merged
merged 79 commits into from
Oct 17, 2024

Conversation

Legend-Master
Copy link
Contributor

@Legend-Master Legend-Master commented Oct 2, 2024

Follow up of #1550

Changes:

New features:

  • Add getStore/get_store share stores across js and rust side
  • Add default (de)serialize functions settings
  • Allow js to use pre-stored (de)serialize functions
  • Add back lazy store (implemented in js)

Default changes:

@Legend-Master Legend-Master requested a review from a team as a code owner October 2, 2024 10:02
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Package Changes Through 8e71393

There are 10 changes which include dialog with patch, dialog-js with patch, positioner with patch, positioner-js with patch, http with patch, http-js with patch, shell with patch, shell-js with patch, store-js with minor, store with minor

Planned Package Versions

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

package current next
api-example 2.0.2 2.0.3
api-example-js 2.0.0 2.0.1
dialog 2.0.1 2.0.2
dialog-js 2.0.0 2.0.1
http 2.0.1 2.0.2
http-js 2.0.0 2.0.1
positioner 2.0.1 2.0.2
positioner-js 2.0.0 2.0.1
shell 2.0.1 2.0.2
shell-js 2.0.0 2.0.1
store 2.0.1 2.1.0
store-js 2.0.0 2.1.0

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

can't we automatically share the store? like always make it available in the same collection, so a call from another language would pick it up anyway?

@Legend-Master
Copy link
Contributor Author

Yeah I was thinking about it, but that means you'll need to always preserve that store, sometimes you might just want to read the data from it and forget about it, maybe share/preserve it by default, and add a close function or something

@Legend-Master
Copy link
Contributor Author

I need to take a break, this thing is haunting me right now 😂

@Legend-Master Legend-Master changed the title refactor(store): more reworks refactor(store)!: more reworks Oct 2, 2024
@Legend-Master Legend-Master marked this pull request as draft October 2, 2024 15:18
@Legend-Master Legend-Master marked this pull request as ready for review October 3, 2024 02:52
@Legend-Master
Copy link
Contributor Author

Legend-Master commented Oct 3, 2024

Note: don't merge it yet, I didn't have the time to test and write the docs yet, got some other things to handle, might be able get back on this tomorrow or something like that

@Legend-Master Legend-Master linked an issue Oct 3, 2024 that may be closed by this pull request
@Legend-Master
Copy link
Contributor Author

To be honest, I still find create to be very confusing, I don't think anyone will expect it to mean open an empty store in the rust side without loading from disk while load gets the opened store or create a new store but also loads it

amrbashir
amrbashir previously approved these changes Oct 17, 2024
@lucasfernog
Copy link
Member

To be honest, I still find create to be very confusing, I don't think anyone will expect it to mean open an empty store in the rust side without loading from disk while load gets the opened store or create a new store but also loads it

I've removed the create function from the Rust side, maybe we do the same on the JS side too. I'll push this change for you to take a look.

@lucasfernog
Copy link
Member

@amrbashir i wanted to use the Resource class close() method instead of creating a dedicated one but I can't because we're using the AppHandle resource table and not the Webview one.. should the resources plugin allow me to configure which table i'm referring to? or fallback to other tables?

@amrbashir
Copy link
Member

I was thinking something similar but Idk if the auditors will approve, maybe if we allow only closing resources from the same webview, or the same window or the app handle, It should be fine then.

@amrbashir
Copy link
Member

I.e. webview can't close resources from other webviews.

@lucasfernog
Copy link
Member

yeah so fallback to the parent window or AppHandle, that should be good

@lucasfernog
Copy link
Member

lucasfernog commented Oct 17, 2024

usually you shouldn't even know the RID if you can't also close the resource.. otherwise users can use a wrapper ID instead

@lucasfernog
Copy link
Member

all good for us to merge and release this? @amrbashir and @Legend-Master
no more breaking changes pls :D

@Legend-Master
Copy link
Contributor Author

Except for close doesn't actually work right now, looks good to me

@lucasfernog
Copy link
Member

we'll fix on the tauri side

lucasfernog added a commit to tauri-apps/tauri that referenced this pull request Oct 17, 2024
this changes the resource plugin close() API to fallback to the parent window and AppHandle resource tables, letting the JS to delete global resources.
The need for this was brought up on tauri-apps/plugins-workspace#1860 (comment)
the store plugin stores the resources in the AppHandle, and we want the existing close() API to work on global resources otherwise every consumer needs their own resource close commands
@Legend-Master Legend-Master merged commit 8c67d44 into tauri-apps:v2 Oct 17, 2024
18 checks passed
lucasfernog added a commit to tauri-apps/tauri that referenced this pull request Oct 17, 2024
…11398)

this changes the resource plugin close() API to fallback to the parent window and AppHandle resource tables, letting the JS to delete global resources.
The need for this was brought up on tauri-apps/plugins-workspace#1860 (comment)
the store plugin stores the resources in the AppHandle, and we want the existing close() API to work on global resources otherwise every consumer needs their own resource close commands
Legend-Master added a commit to tauri-apps/tauri-docs that referenced this pull request Oct 21, 2024
Sir-Thom pushed a commit to Sir-Thom/plugins-workspace that referenced this pull request Oct 22, 2024
* refactor(store): more reworks

* Enable auto save by default

* Store to resource table by default

* Remove share store

* Clean up

* Add close store

* Add store function

* Add lazy store

* Add init to lazy store

* refresh cache in example

* Add get-or-create-store

* Revert "Add get-or-create-store"

This reverts commit 7ffd769.

* try get first

* Docs

* Use absolute path for store

* more docs

* Allow js to use pre-stored (de)serialize functions

* Fix js get and close store

* Show case how to use pretty json

* Update readme

* Use store instead of `store_builder` in example

* Build

* Fix example

* More docs for StoreBuilder::build

* Add default (de)serialize fn

* Use pretty json by default

* Use `undefined` for empty value in get

* Change files

* Differentiate json null from no value for events

* Add create or existing

* Build

* Rename inner store's inset method to set

* Update readme

* Apply suggestions from code review

* Use close instead

* Update breaking changes

* Return result in resolve_store_path

* Change to close_resource and take &self

* Clean up

* Apply suggestions from code review

Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>

* Remove unused pub(crate)

* Update change file

* Expose resolve_store_path

* Remove with_store

* Remove StoreInner from pub and expose is_empty

* Fix wrong jsdoc param

* Update readme

* rename createOrExistingStore to createOrLoad

* make api consistent with the JS implementation, add examples

* fmt

* reintroduce "get existing store" behavior for create_or_load

* rename createOrLoad to newOrExisting

* keep store lock throughout whole new_or_existing_inner

* Remove load

* Missed if load

* Don't make StoreState public

* Remove R: Runtime from Builder

* rename newOrExisting to load, load to reload

* update docs

* rename missing reload fn

* rename builder fn to build()

* fix default permission

* Fix description and create_new logic

* Clippy

* Update docs

* Update docs

* remove create_store command

* remove close_store command since we extend from Resource

* Revert "remove close_store command since we extend from Resource"

This reverts commit 4a29fc8.

* Reapply "remove close_store command since we extend from Resource"

This reverts commit 70a1830.

---------

Co-authored-by: Amr Bashir <amr.bashir2015@gmail.com>
Co-authored-by: Lucas Nogueira <lucas@tauri.app>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants