Fix a regression from #4235. #4265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`fetchWithAuth`'s implementation is very short, and we don't use it anywhere except `apiFetch`, so it already seems wise just to inline it. We're also about to shorten `apiFetch` by using a URL computation instead of string concatenation (for zulip#4146), which will make an even stronger case for inlining `fetchWithAuth`.
chrisbobbe
force-pushed
the
pr-url-fix
branch
from
2e0fe44 to
365e669
Compare
|
Thanks! About to merge. I think this isn't a problem with |
For zulip#4146, but also to fix a recent regression from 865914f. The URL constructor already throws a special "Invalid URL" error if it can't put together a valid URL from the inputs, so, nicely, we get to remove the `isValidUrl` call site. There was a double-forward-slash issue in this string concatenation: the realm gained a trailing slash with the round-trip through `new URL(…).toString()` from 865914f, and we kept the leading slash in what we were adding to the end of the realm.
And use a URL computation instead, which is what we do elsewhere. In particular, it's how we check whether we can store the realm in Redux, so it's good to make the UI align with that. This helper function is a relic from the days before zulip#4081 and zulip#4146, so it should disappear anyway. It was the sole user of an NPM package. So, uninstall that.
|
Thanks for the review and merge! |
|
It seems likely that this line has the same issue, then, in the Apple auth flow: |
|
Ah, yeah, probably—testing with a fix now. |
OK, just sent #4266. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was an accidental duplication of the trailing slash on the realm in all API requests, starting in 865914f. So, fix that and make some more progress toward #4146.