[5.4] Refactoring mix method (bis). #19666

mathieutu · 2017-06-19T08:02:43Z

Some features added, and code style fixed from reviews of the previous PR:
#19639

@taylorotwell have fun, I'm waiting for your wisdom!
😊

morloderex · 2017-06-19T09:48:01Z

src/Illuminate/Foundation/helpers.php

-        }
-
-        return new HtmlString($manifestDirectory.$manifest[$path]);
+        return app('mix')->mix($path, $manifestDirectory);


I think it looks strange to do (new Mix)->mix() why not something more like (new Mix)->create()..

Yep or generate maybe?

Yeah I thought about that.
create and generate aren't good ones either because you don't create or generate anything, you just find the good path in a file, so find? I didn't find something I liked, so I kept that because in any way, the method will always be called by the mix() helper, as usual.

What about load?

Could use resolve(). If Taylor merges it, he could always rename it.

I love resolve thank you! 🎉

mathieutu · 2017-06-19T10:27:21Z

tests/View/Mix/MixTest.php

+{
+    public function setUp()
+    {
+        app()->singleton('path.public', function () {


🤔
Yeah, it's true and I also could have created all the other files by the same way actually (file_put_content(json_encode(...))), but I've done it like that.

I, personally, prefer commit two empty files to test a function than add some extra complexity to generate them.

But, anyway, does it really matter ??

lucasmichot · 2017-06-19T12:07:55Z

src/Illuminate/View/Mix/Mix.php

+     */
+    protected function getManifest()
+    {
+        if (! $this->manifest) {


No need to go further if $this->manifest is not empty. You can favor an early return:

if ($this->manifest) { return $this->manifest; } if (! file_exists($manifestPath = public_path($this->manifestDirectory.$this->manifestFilename))) { throw new MixException('The Mix manifest does not exist.'); } $this->manifest = json_decode(file_get_contents($manifestPath), true); if (json_last_error() === JSON_ERROR_NONE) { return $this->manifest; } throw new MixException("The Mix manifest isn't a proper json file.");

It's your code right here, you made me change mine to yours for no valid reason, and now you're again changing your mind. It's just about a basic refactoring and you're commenting every line I'm writing for 3 days each time I make a new commit.

I have work to do, and I haven't not enough time to satisfy all your dreams on a basic function like this one. So, I don't know what's your goal, but please stop, or do it yourself!

It's just about a basic refactoring

It's a great PR but a critical function.

lucasmichot · 2017-06-19T12:08:24Z

src/Illuminate/View/Mix/Mix.php

+     *
+     * @param  string  $manifestFilename
+     *
+     * @return Mix


We use @return $this

lucasmichot · 2017-06-19T12:08:31Z

src/Illuminate/View/Mix/Mix.php

+     *
+     * @param  string  $hmrFilename
+     *
+     * @return Mix


We use @return $this

lucasmichot · 2017-06-19T12:12:17Z

tests/View/Mix/MixTest.php

+
+    /**
+     * @expectedException \Illuminate\View\Mix\MixException
+     */


The exception message should also be tested here.

Nope, we don't care about the message, it's just to check that is enabled.

We care as your test fails with @expectedExceptionMessage Mix is disabled
If the same exception is used for different purposes within a context, then the exception message should be tested too.

No, not at all!
This exception message doesn't exists, we are not expecting any special exception, just check the mix method is enabled.
Please read the code instead of commenting every line I'm writing!

I've read it for some days already - I think that I got the picture
When you run $this->getMix()->disable()->enable()->mix('foo.css'); is thrown \Illuminate\View\Mix\MixException which is tested, and this exception comes with the message The Mix manifest does not exist.
So this exception message should be also tested.

Ok, we don't suceed to understand each other, I'm just switching in our mutual native language, it will be easier for us. 😕

Ici on s'en fout de savoir ce qui arrive après, on veut juste s'assurer que la méthode enable "marche" comme il faut. Dès l'instant où il y a autre chose que le comportement de disable, donc une exception, peu importe laquelle dans ce cas là, ça veut dire que ça marche.

On peut rajouter le test du message, mais c'est une question de logique ici, on ne cherche pas à savoir quel message va être affiché, juste s'assurer que la fonction continue, et ne s'arrête pas sur le disable.

lucasmichot · 2017-06-19T12:44:33Z

tests/View/Mix/MixTest.php

+
+    private function getMix()
+    {
+        return new \Illuminate\View\Mix\Mix;


No need to declare a useless method here.
$this->mix = new \Illuminate\View\Mix\Mix; can be in setUp method.

Guy, I think you should make all the PRs just by yourself.

It's just about a basic refactoring, I've added here tests that didn't exist and you're commenting every line I'm writing for 3 days each time I make a new commit.

I have work to do, and I haven't not enough time to satisfy all your dreams on a basic function like this one. So, I don't know what's your goal, but please stop, or do it yourself!

I believe there is some misunderstanding out there.
You plan to refactor a mix function that everyone is using on 5.4 impacting current users, and that will be soon merged to 5.5.
This is a function loaded on each page request displaying a view, so it's expected to be performant and highly tested, right?

No need to be pissed and get personal

Yeah, but you can understand that this is much better than before, and you agree that each commit I'm making, you add some things.
I'm ok to discuss, exchange some "good" practices, and I agree than I have not your experience about the coding style of this framework. It's why I used your advices and pieces of code, even if sometimes I don't agree with.

But please if we can just tell everything once, and don't come back each time on the same piece of code, we will save time both. Especially like here when it's here in a test, than even didn't exist!!

Imho you will have a lot more value to write tests than just make PRs to rewrite docblocks and code style of others!

I personally have a job, make opensource on my free time because I love it, and you are just disgusting me about that and it's a bit too bad...

If you want, you can make a PR to my branch I will merge it immediately, and so it's ok we're done and we can move to important stuffs!

Hey Mathieu, pas de problème ;-)
I have a job too, and do this on my free-time too - I am just making sure that you're great PR will be merged and ready to rock

Next time I will PR you fork if you feels that easier, up to you

Ok Lucas.
So, I'll do all the modifications tonight. Do you see now something else to update? I think it will be the last time, this is THE moment to tell me! 🙃

mathieutu · 2017-06-20T08:03:23Z

@lucasmichot it's ok for you?
Can I ping taylor to merge it?

ntzm · 2017-06-22T10:39:13Z

tests/View/Mix/MixTest.php

+
+    public function testMixMethodWhenDisabled()
+    {
+        $this->assertEquals(new HTMLString('Mix is disabled!'), $this->mix->disable()->resolve('foo.css'));


Casing of HTMLString should be HtmlString

Right! thx!

mathieutu · 2017-06-23T12:06:07Z

@taylorotwell I think we're ok, you can merge it!
🎉

taylorotwell · 2017-06-25T14:30:38Z

The object design on this seems weird. Why does the Mix object have this stateful $path variable and this init method that sets all this state? Shouldn't you just be passing a path in and getting a path out? I don't understand why it's hanging onto all this information.

devcircus · 2017-06-25T15:00:40Z

At the end if the day, make sure we have a way to use a custom manifest file name.

sebastiandedeyne · 2017-06-26T08:00:17Z

First off, really hope to see something like this merged in since it could drastically reduce build times on CI environments.

If variables like $hmrFilename and $manifestFilename really are things people care about modifying, maybe mix should get it's own config file? I think that'd make more sense than all these getters and setters, no?

lucasmichot · 2017-06-26T14:10:17Z

@sebastiandedeyne
hmr is hardcoded : https://github.com/JeffreyWay/laravel-mix/blob/bf33ae295d8713a1b8625e79ae9059094ecc53e5/src/Mix.js#L62

But mix-manifest.json can be customized, https://github.com/JeffreyWay/laravel-mix/blob/9a3426b7ba2d317a4ec9c59d7051484a2d7852fc/src/Manifest.js#L12

lucasmichot · 2017-06-26T14:12:33Z

@mathieutu , I believe this PR should be updated to comply #19764 which has been merged recently

taylorotwell · 2017-06-28T18:32:18Z

See above comment about conflicts and please send this to 5.5 and I will try to rewrite some of it. Thanks.

mathieutu · 2017-07-04T14:06:16Z

@lucasmichot We agree that the PR you mentioned can't actually be working at all?!

see #19764 (comment)

I've fixed it in #19895.

mathieutu added 7 commits June 16, 2017 17:01

Refactoring mix()

66ddbca

Add the possibility to disable the mix function.

ddbad7a

Add all the tests for mix().

e683b62

Update tests and fix bug with data_get()

c31cc24

fix style

6fa8a17

Update with PR review comments (laravel#19639).

b6e2d33

Style CI.

5de7ef5

morloderex reviewed Jun 19, 2017

View reviewed changes

mathieutu commented Jun 19, 2017

View reviewed changes

And again, some reviews: remove empty file from commits…

7127ec3

lucasmichot reviewed Jun 19, 2017

View reviewed changes

lucasmichot and others added 3 commits June 19, 2017 16:13

Fix laravel#19666

5111036

Fix last review.

f8fb58b

Change method name.

1828cc4

ntzm reviewed Jun 22, 2017

View reviewed changes

fix Case

c16c217

taylorotwell closed this Jun 28, 2017

mathieutu mentioned this pull request Jul 4, 2017

[5.5] Mix refactoring #19895

Closed

[5.4] Refactoring mix method (bis). #19666

[5.4] Refactoring mix method (bis). #19666

Conversation

mathieutu commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

mathieutu Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

lucasmichot Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu commented Jun 23, 2017 • edited Loading

taylorotwell commented Jun 25, 2017 • edited Loading

devcircus commented Jun 25, 2017

sebastiandedeyne commented Jun 26, 2017

lucasmichot commented Jun 26, 2017

lucasmichot commented Jun 26, 2017

taylorotwell commented Jun 28, 2017

mathieutu commented Jul 4, 2017

mathieutu Jun 19, 2017 •

edited

Loading

lucasmichot Jun 19, 2017 •

edited

Loading

lucasmichot Jun 19, 2017 •

edited

Loading

mathieutu Jun 19, 2017 •

edited

Loading

mathieutu Jun 19, 2017 •

edited

Loading

lucasmichot Jun 19, 2017 •

edited

Loading

mathieutu commented Jun 23, 2017 •

edited

Loading

taylorotwell commented Jun 25, 2017 •

edited

Loading