-
Notifications
You must be signed in to change notification settings - Fork 247
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
Remove baseProtocol and baseDomain #337
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I marked two places where baseUrl
is new in this PR just to highlight the order of PR merging with #338. Otherwise good to go 👍
$this->baseDomain = $url['host'] ?? ''; | ||
$this->basePort = $url['port'] ?? null; | ||
}); | ||
$this->basePort = parse_url($this->baseUrl)['port'] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanna make sure, same as the other PR, that this gets updated to $this->url
in whatever order these PRs are merged. Not necessarily here, probably in #338, but don't wanna forget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -28,7 +28,7 @@ class Route { | |||
// If we're building just a path there's no origin, otherwise: if this route has a | |||
// domain configured we construct the origin with that, if not we use the app URL | |||
const origin = !this.config.absolute ? '' : this.definition.domain | |||
? `${this.config.baseProtocol}://${this.definition.domain}${this.config.basePort ? `:${this.config.basePort}` : ''}` | |||
? `${this.config.baseUrl.match(/^\w+:\/\//)[0]}${this.definition.domain}${this.config.basePort ? `:${this.config.basePort}` : ''}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this.config.url
* Add upgrade guide * Wip * Wip * Update GitHub actions to not run on Markdown files * Wip * Formatting * Add changes in #337 * Add anchor links * Add changes in #338 * Add changes in #341, formatting * Wip * Update usage examples in Readme * Move CSP section down * LICENSE.md → LICENSE * Wip * Wip * Formatting * Add new features section to Upgrading * Prep Changelog headers and tags for v1 * Move current() with query fix to Fixed section * Add entries for #334 and #344 to Upgrading * Add Upgrading entry for check() being deprecated * Add #345 to Upgrading * Wip on Readme * JavaScript → Javascript * Wording/formatting * Update Readme: - Move Usage above Setup - Remove 'basic setup' section about @routes directive, it's covered in Installation - Wording fixes - Add note about boolean encoding (see #345) - Remove old 'Artisan Command' section * Formatting * Formatting and wording * Javascript → JavaScript * Remove unused heading link * Add section under 'Other' for setting up an API endpoint to return routes, link to that from SPA and JS sections * Fix wording re: watching files * :)
This PR removes the
baseProtocol
andbaseDomain
properties on the Ziggy configuration object.baseDomain
actually wasn't used anywhere, andbaseProtocol
was inferred frombaseUrl
, so now we just use that directly.Closes #333.