Skip to content

refactor: remove internal and_then chaining - #1556

Merged
amrbashir merged 11 commits into
tauri-apps:devfrom
Legend-Master:remove-and-then-chain
May 24, 2025
Merged

refactor: remove internal and_then chaining#1556
amrbashir merged 11 commits into
tauri-apps:devfrom
Legend-Master:remove-and-then-chain

Conversation

@Legend-Master

@Legend-Master Legend-Master commented May 10, 2025

Copy link
Copy Markdown
Contributor

I noticed this while debugging the compile time in tauri with cargo-llvm-lines

image

The and_then chaining was introduced in #1384 to record the duplcated custom protocol error

If we would return it in build anyways, let's just check it there instead

@Legend-Master
Legend-Master requested a review from a team as a code owner May 10, 2025 14:32
@github-actions

github-actions Bot commented May 10, 2025

Copy link
Copy Markdown
Contributor

Package Changes Through 2ef05ba

There are 1 changes which include wry 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
wry 0.51.2 0.52.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

@FabianLars

Copy link
Copy Markdown
Member

Sometimes(?) simple is better :D

@FabianLars

Copy link
Copy Markdown
Member

I'm only on my phone rn so I may be imagining things but if we use hash map.insert with the protocol name as key we will silently over duplicates on the with_protocol method, no?

@Legend-Master

Copy link
Copy Markdown
Contributor Author

but if we use hash map.insert with the protocol name as key we will silently over duplicates on the with_protocol method, no?

The newer handler will override the existing one in the builder, but I feel like that's what makes sense to me, since it's a builder that's not being built yet, why not to allow that?

@FabianLars

Copy link
Copy Markdown
Member

I'm okay with that, it's just different than it was with and_then 🤷 also idk if your argument really applies to a builder method you'll often call multiple times without wanting to overwrite the protocol. but yeah, either way works for me, just wanted to make sure it wasn't an accident.

FabianLars
FabianLars previously approved these changes May 11, 2025

@amrbashir amrbashir left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't merge just yet, I will review it as soon as I can

Comment thread src/error.rs
Comment thread src/lib.rs
Comment thread src/web_context.rs
Comment thread src/webkitgtk/web_context.rs Outdated
Comment thread src/lib.rs Outdated
@Legend-Master

Copy link
Copy Markdown
Contributor Author

The reason I put the linux web context check inside the final build is because I think it makes more sense to check it just before the build since it doesn't matter before that point

Anyways, that's fine either ways

@amrbashir

amrbashir commented May 24, 2025

Copy link
Copy Markdown
Member

The reason I put the linux web context check inside the final build is because I think it makes more sense to check it just before the build since it doesn't matter before that point

I moved it there so we can find the logic more easily, it doesn't make sense to delay the check until .build call.

@amrbashir
amrbashir merged commit f868658 into tauri-apps:dev May 24, 2025
@Legend-Master
Legend-Master deleted the remove-and-then-chain branch May 24, 2025 13:47
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