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

Added support for multiple manifest files #19764

Merged
merged 3 commits into from
Jun 26, 2017
Merged

Added support for multiple manifest files #19764

merged 3 commits into from
Jun 26, 2017

Conversation

shedokan
Copy link
Contributor

Currently if you try to use two different manifest files, only the first one is cached and the second one breaks.

<script src="{{ mix('app.js') }}"></script>
<script src="{{ mix('comments.js', 'vendor/comments') }}"></script> <-- Breaks

This code caches all of the manifest files, to allow all of them to be used together.

@taylorotwell taylorotwell merged commit 6c0e2f8 into laravel:5.4 Jun 26, 2017
@mathieutu
Copy link
Contributor

Actually for me this code can't work at all! ^^"
You check if the manifestKey is in_arraybut add it later as a key of the array.

@taylorotwell I really think we shouldn't accept code which is not tested.
This piece of code is useless.

I've fixed it in #19895 but we should also fix it in 5.4...

@georgehanson
Copy link
Contributor

@mathieutu I can't see the issue here? It checks to see if the manifestKey is cached and if it is gets the current cached value and assigns it to a variable. Otherwise, it then puts the manifestKey into the cache.

Is this not the intended behaviour? It seems right to me.

@taylorotwell
Copy link
Member

Is this broken or not? Getting weary of these Mix changes.

@taylorotwell
Copy link
Member

@JeffreyWay

@JeffreyWay
Copy link
Contributor

I think it's strange that people want multiple mix-manifest.json files in the first place. What's the use case here?

@bobbybouwmann
Copy link
Contributor

@JeffreyWay If I remember correctly some people wanted to generate multiple files within mix. For example one for the application and one for specific pages and maybe one for vendor resources.

@JoostK
Copy link
Contributor

JoostK commented Jul 4, 2017

I agree with @mathieutu, the caching part can't work now.

if (file_exists(public_path($manifestDirectory.'/hot'))) {
return new HtmlString("//localhost:8080{$path}");
}

if (! $manifest) {
if (in_array($manifestKey, $manifests)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be array_key_exists($manifestKey, $manifests)?

@mathieutu @georgehanson @taylorotwell

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah of course, it's why I've seen this can't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my bad

@mathieutu
Copy link
Contributor

@georgehanson @taylorotwell If you look at the code, you will directly see that this can't work.
as I said and @djtarazona showed, the $manifestKey is added as a key of $manifests and the check is done in the values of the array. We have to use array_key_exists here.

The mix method will continue to work globally, but it will no longer have a cache system.
If you test it with code coverage, you will directly see that this piece of code is never used.

Testing guy, testing!!

I'll do a PR to fix that if @taylorotwell really doesn't want to merge my PR of global mix refactoring in 5.4...

PS1: I don't use multiple file either, but this is in the code, and asked for by some people in comments, so...
PS2: I'm really getting weary of all of that too. Initially just proposed a refactoring to make thing more clearer, add some highly demanded new features and add lot of tests 2 weeks ago, and we're still discussing about that...

@joshmanders
Copy link
Contributor

@bobbybouwmann the manifest file is just meant to list all the artifacts of the build, all of the resources should be in there, not separating them into their own manifest files.

@shedokan
Copy link
Contributor Author

shedokan commented Jul 5, 2017

@JeffreyWay @bobbybouwmann @joshmanders @mathieutu

  1. I first needed this when I used a 3rd party plugin that ships with compiled assets, with their own manifest file
  2. Also needed this when compiling with mix to separate js files for client and admin(with vendor extraction) which is not possible unless I build to two targets, using two different manifest files

@devcircus
Copy link
Contributor

Separate js output files and vendor extraction shouldn't require separate manifests. That's a very common case.

@shedokan
Copy link
Contributor Author

shedokan commented Jul 5, 2017

@devcircus It were some issues with an es6 librarie, so I had to disable uglify for the admin code.
Either way, it's not the place to solve my mix issues, but the 3rd party plugin is more than enough to need such a feature.

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.

10 participants