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

feat(api): add SERIALIZE_TO_IPC_FN const and implement it for dpi types, add more constructors #11191

Open
wants to merge 23 commits into
base: dev
Choose a base branch
from

Conversation

amrbashir
Copy link
Member

No description provided.

@amrbashir amrbashir requested a review from a team as a code owner October 2, 2024 02:54
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Package Changes Through 258fe72

There are 5 changes which include @tauri-apps/api with minor, tauri with minor, tauri-bundler with patch, tauri-cli with patch, @tauri-apps/cli with patch

Planned Package Versions

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

package current next
@tauri-apps/api 2.0.3 2.1.0
tauri-bundler 2.0.4 2.0.5
tauri 2.0.6 2.1.0
@tauri-apps/cli 2.0.4 2.0.5
tauri-cli 2.0.4 2.0.5

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


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

@amrbashir amrbashir marked this pull request as draft October 2, 2024 03:43
@amrbashir amrbashir changed the title feat(api): add toIpc method on dpi types, add more constructors feat(api): add toIpc and toJSON method on dpi types, add more constructors Oct 2, 2024
@amrbashir amrbashir marked this pull request as ready for review October 2, 2024 06:13
packages/api/src/dpi.ts Outdated Show resolved Hide resolved
packages/api/src/dpi.ts Outdated Show resolved Hide resolved
packages/api/src/dpi.ts Outdated Show resolved Hide resolved
packages/api/src/dpi.ts Outdated Show resolved Hide resolved
@lucasfernog
Copy link
Member

this one can wait to be merged after the v2 stable release right?

packages/api/src/window.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

Do you think we should use toTauriIpc instead of just toIPC?

packages/api/src/window.ts Show resolved Hide resolved
crates/tauri/scripts/process-ipc-message-fn.js Outdated Show resolved Hide resolved
@amrbashir
Copy link
Member Author

amrbashir commented Oct 3, 2024

Do you think we should use toTauriIpc instead of just toIPC?

yes, i am not to keen on toIPC and don't want to make it collide with user types, so I was thinking maybe IpcValue class to extend like Resource class.

@amrbashir
Copy link
Member Author

Made a ToIpcSymbol const and used that instead.

@Legend-Master
Copy link
Contributor

Legend-Master commented Oct 3, 2024

Sounds good to me, but shouldn't the symbol be a Symbol not a string?

@amrbashir
Copy link
Member Author

amrbashir commented Oct 3, 2024

Sounds good to me, but shouldn't the symbol be a Symbol not a string?

Normally it should be, but we access this value in multiple places and we can't keep re-defining it because it will not be the same value (that's how Symbol works, it is always unique). We can define it on the global window object and access from different places but so I don't want to expose this symbol in core module like this

const ToIPCSymbol = window.ToIPCSymbol

because it will break usage with next.js and SSR framworks and I don't want to deal with their issues again.

@Legend-Master
Copy link
Contributor

Shouldn't this work?

image

@Legend-Master
Copy link
Contributor

Never mind, we can't use import here

@Legend-Master
Copy link
Contributor

Legend-Master commented Oct 3, 2024

What about Symbol.for()?

@amrbashir
Copy link
Member Author

amrbashir commented Oct 3, 2024

That works I guess but is it really that different? the main problem here IMO is that we can't share the value by using imports

@Legend-Master
Copy link
Contributor

Right, that doesn't really make a difference, let's just go with a string then, in that case maybe changing the name to TO_IPC_KEY instead since it's not really a symbol?

@amrbashir
Copy link
Member Author

renamed!

Legend-Master
Legend-Master previously approved these changes Oct 3, 2024
Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

Didn't test it, but the code looks good to me

*
* @since 2.0.0
*/
[SERIALIZE_TO_IPC_FN]() {
Copy link
Member

Choose a reason for hiding this comment

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

I like the whole toIPC approach (makes the __CHANNEL__ thingy available to anyone) but this is actually a breaking change - existing commands relying on PhysicalPosition / PhysicalSize / LogicalPosition / LogicalSize cannot deserialize this new format, so we should revert these - if you explictly want a generic Position or Size type we need to create a wrapper type just like Rust has one

Copy link
Member Author

Choose a reason for hiding this comment

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

that makes sense, I have added Position and Size classes and reverted breaking changes.

@@ -26,8 +76,6 @@ function transformCallback<T = unknown>(

class Channel<T = unknown> {
id: number
// @ts-expect-error field used by the IPC serializer
private readonly __TAURI_CHANNEL_MARKER__ = true
Copy link
Member Author

Choose a reason for hiding this comment

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

@lucasfernog isn't this a breaking change? removing the field will make new @tauri-apps/api packages not work with [email protected]

Comment on lines -28 to -32
val instanceof Object &&
'__TAURI_CHANNEL_MARKER__' in val &&
typeof val.id === 'number'
) {
return `__CHANNEL__:${val.id}`
Copy link
Member Author

Choose a reason for hiding this comment

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

same here

@amrbashir amrbashir changed the title feat(api): add toIpc and toJSON method on dpi types, add more constructors feat(api): add SERIALIZE_TO_IPC_FN const and implement it for dpi types, add more constructors Oct 18, 2024
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.

3 participants