Parse url for base branch and pr bases

[mtsgrd]

• will help us compare pr base against target

---

This is part 1 of 2 in a stack made with GitButler:

•  2  Disable merge button if pr base is not target branch #5612
•  1  Parse url for base branch and pr bases #5611 👈

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
gitbutler-components	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 19, 2024 7:33pm

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
gitbutler-web	⬜️ Skipped (Inspect)			Nov 19, 2024 7:33pm

[mtsgrd]

@Caleb-T-Owens what do you think of this the UniqueDerived class? I think we discussed this last week.

[krlvi]

didnt properly review this but if it sucks we can fix it tomorro

[Caleb-T-Owens]

I'm not sure the class is helpful, can't we just say:

export function shallowDeduplicate<A>(store: Readable<A>): Readable<A> {
	const deduplicatedStore = readable(get(store), (set) => {
		let lastValue = get(store);
		const unsubscribe = store.subscribe((value) => {
			if (!shallowCompare(lastValue, value)) {
				lastValue = value;
				set(value);
			}
		});
		return unsubscribe;
	});

	return deduplicatedStore;
}

[mtsgrd]

Let me try that out. *edit: one immediate thing I would want to avoid is subscribing to the store when creating the derived store, which happens here because of the get() method.

[Caleb-T-Owens]

You should be safe to set the readable initial value to undefined, and keep the result as Readable<T> because the start stop notifier gets run synchronously

[Caleb-T-Owens]

Hmm... or even write it as a derived to make that intent clearer

[Caleb-T-Owens]

export function shallowDeduplicate<A>(store: Readable<A>): Readable<A> {
	let lastValue: undefined | A = undefined;
	const deduplicatedStore = derived<Readable<A>, A>(store, (value, set) => {
			if (!shallowCompare(lastValue, value)) {
				lastValue = value;
				set(value);
			}
	});

	return deduplicatedStore;
}

[Caleb-T-Owens]

AHHhhhh no... that derived version isn't safe

[Caleb-T-Owens]

undefined needs to be considered here

[Caleb-T-Owens]

Why don't we want these tests?

[mtsgrd]

Because they existed for exactly 1 location in the code asking "is the protocol http?". The if condition can just as well be protocol.startsWith('http') rather than having a specialized and tested function.

[Caleb-T-Owens]

didnt properly review this but if it sucks we can fix it tomorro

I'm bothered by this. Especially when I was asked for feedback.