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

Don't wrap mix path with asset() #20182

Closed
wants to merge 3 commits into from

Conversation

dwightwatson
Copy link
Contributor

This removes the asset function from wrapping the asset path. I've created a similar fix before (##17727) and it was re-introduced in #19968 which broke one of my apps, and looks like it may have affected some other developers too.

This allows developers to prefix the asset path with whatever host they like and if they choose not to it'll just be a relative path on their own application, so it should work just the same.

@dwightwatson
Copy link
Contributor Author

I see this has actually been done already (#20165), sorry about the duplication.

I'll leave this open as it adds a test for the expected functionality in case you're a fan of that. Would hopefully prevent this from happening again.

@themsaid
Copy link
Member

I'm not accepting any Mix changes from anyone accept @JeffreyWay for at least a month.

Quoting Taylor in #20165

@themsaid themsaid closed this Jul 20, 2017
@dwightwatson
Copy link
Contributor Author

@themsaid the only outstanding change of this PR (the change to Laravel Mix has already been made upstream) is the test that it adds. I think that's worth considering so we don't break this feature again.

@themsaid
Copy link
Member

The PR you mention #20165 was reverted, your PR introduces the same changes plus a test, so I'm afraid this won't be merged as well. sorry :)

@dwightwatson
Copy link
Contributor Author

Oh gotcha, didn't realise that PR was since reverted too. Shame as it does break backwards compatibility and we're going to have to make changes to manually strip out the domain name from the output of the method. No worries, I appreciate you getting back to me.

@mathieutu
Copy link
Contributor

@dwightwatson I agree with you. I didn't expect this weird usage of mix method.

I'm for deleting the asset method too and, so, for merging your PR (even more because of the tests), but Taylor's reactions about all this mix changes are just... really unsettling, and disappointing.
I'm afraid we can't do anything anymore 😥.

@it-can
Copy link
Contributor

it-can commented Jul 20, 2017

@JeffreyWay can you please help us with this one?

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.

4 participants