[5.4] Refactoring mix method. #19639

mathieutu · 2017-06-16T17:27:59Z

Hi folks,
for this 4th PR of the week, I worked on the mix() method!

I totally refactored it in a proper testable service, called by app('mix').
I also fully tested it.

And then, I've managed the Exception thrown when you execute the tests on views where mix() is called, and you don't have installed and launch js stuffs.

We discussed about that in laravel/ideas#634.

So the result is:

Without the PR:
With the PR:

I haven't made a Facade for that, imo it's enough easy to use like that. We can add one if you want.

I'm waiting for your comments!
Matt'

joshmanders · 2017-06-16T18:02:18Z

src/Illuminate/View/Mix/Mix.php

+     */
+    protected function hmrPath()
+    {
+        return new HtmlString("//localhost:8080{$this->path}");


Be nice to be able to configure this port and host.

I don't know if you can really modify that on laravel-mix.. 🤔 I don't see it in docs.
@JeffreyWay ?

lucasmichot · 2017-06-16T18:49:21Z

src/Illuminate/View/Mix/Mix.php

+     */
+    protected function sanitize($path)
+    {
+        if ($path && ! starts_with($path, '/')) {


Use Str::startsWith instead

It's the same just shorter..
But I've no problem with that if you have a good reason 🙂

Within the framework, we don't use array and string helper, we directly call Str and Arr functions.

lucasmichot · 2017-06-16T18:49:35Z

src/Illuminate/View/Mix/Mix.php

+
+class Mix
+{
+    protected $path;


4 missing docblocks

Right! I missed them...
Thank you!

lucasmichot · 2017-06-16T18:56:53Z

src/Illuminate/View/Mix/Mix.php

+                throw new MixException('The Mix manifest does not exist.');
+            }
+
+            $this->manifest = collect(json_decode(file_get_contents($manifestPath), true));


Is there any point to use a Collection instead of an array?

Just to be able to make a get line 131. A data_get can't be used because of the dot notation (and the extension of files)

Then you can use Arr::get instead, no need of a collection

@lucasmichot no you can't use Arr::get here, because it uses "dot" notation, and it will fail with the extension of the assets:

// src/Illuminate/Support/Arr.php:277 foreach (explode('.', $key) as $segment) { // 'foo.css' => ['foo', 'css']

It's why I used a collection, where the get method doesn't use dot notation.

But a collection is useless:

// getManifest() $this->manifest = json_decode(file_get_contents($manifestPath), true); // getPathFromManifest() if (! array_key_exists($this->path, $manifest = $this->getManifest())) { throw new MixException( "Unable to locate Mix file: {$this->path}. Please check your ". 'webpack.mix.js output paths and try again.' ); } return $manifest[$this->path];

Just as it was done in the mix() helper function

What happens if the manifest file cannot be decoded properly? A test on json_last_error() !== JSON_ERROR_NONE should be done and MixException thrown

lucasmichot · 2017-06-16T18:59:30Z

tests/View/Mix/MixTest.php

+
+    public function testMixMethodThrowAnExceptionIfMixManifestDoesNotExist()
+    {
+        $this->expectException(MixException::class);


In the rest of the tests, we use docblocks annotation to tests exceptions:

/** * @expectedException \Exception * @expectedExceptionMessage The exception message */

Right, I'm seeing it. I prefer the other way (more consistent imho), but no problem with that.

taylorotwell · 2017-06-16T22:55:50Z

What new functionality is there? What is this fixing?

m1guelpf · 2017-06-16T22:59:38Z

@taylorotwell See laravel/ideas#634

mathieutu · 2017-06-17T09:11:04Z

@taylorotwell, TL;DR by order of importance:

Possibility of disable mix() to avoid exception when you don't give a **** of assets. (see my screens above)
Full tested
Refactored to his own proper class and not all the logic in one little helper.

lucasmichot · 2017-06-16T19:07:31Z

tests/View/Mix/MixTest.php

+        });
+    }
+
+    public function tearDown()


Useless, you don't use Mockery.

Right!
Just a bad copy-paste -_-"

lucasmichot · 2017-06-16T19:09:12Z

src/Illuminate/View/Mix/Mix.php

+     */
+    protected function getPathFromManifest()
+    {
+        return $this->getManifest()->get($this->path, function () {


I would use a if statement, instead of passing a closure for the default argument

I like this way to do it, imo it's more in the Laravel spirit.
It's just about pretty code, no more.

Not really, why would you call value() on the closure ?

Actually it comes directly from examples and good practices from Jeffrey and Adam! ^^"
(but as it's just about coding style and I don't really care, I am refactoring that just for you..!)

lucasmichot · 2017-06-16T19:10:59Z

src/Illuminate/View/Mix/Mix.php

+    /**
+     * Get a sanitized version of a path.
+     *
+     * @param  string  $path


string|null as you check that it's not null

nope, actually it can't be null. It's just in case of empty string (the same than before).

lucasmichot · 2017-06-17T11:04:31Z

src/Illuminate/View/Mix/Mix.php

+     */
+    public function enable($enabled = true)
+    {
+        $this->disable(! $enabled);


It's faster to directly set the property: $this->disabled = ! $enabled;

lucasmichot · 2017-06-17T11:05:17Z

src/Illuminate/View/Mix/Mix.php

+     *
+     * @return \Illuminate\Support\HtmlString
+     *
+     * @throws \Illuminate\View\Mix\MixException


This function does not throw any exception

lucasmichot · 2017-06-17T11:07:01Z

src/Illuminate/View/ViewServiceProvider.php

+    public function registerMix()
+    {
+        $this->app->singleton('mix', function () {
+            return new Mix();


new classes without parameters are declared without the braces: return new Mix;

lucasmichot · 2017-06-17T11:08:30Z

tests/Foundation/FoundationHelpersTest.php

+            m::mock(\Illuminate\View\Mix\Mix::class)->shouldReceive('mix')->andReturn('bar')->getMock()
+        );
+
+        $this->assertEquals('bar', mix('foo'));


Better to be stricter and use assertSame as we expect a string.

lucasmichot · 2017-06-17T11:09:16Z

tests/View/Mix/MixTest.php

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


return new \Illuminate\View\Mix\Mix;

lucasmichot · 2017-06-17T11:17:45Z

src/Illuminate/Foundation/helpers.php

@@ -523,40 +523,11 @@ function method_field($method)
     * @param  string  $manifestDirectory
     * @return \Illuminate\Support\HtmlString
     *
-     * @throws \Exception
+     * @throws \Illuminate\View\Mix\MixException


This function does not throw any exception.

lucasmichot · 2017-06-17T11:17:58Z

src/Illuminate/View/Mix/Mix.php

+     * @param  string  $manifestDirectory
+     * @return \Illuminate\Support\HtmlString
+     *
+     * @throws \Illuminate\View\Mix\MixException


This function does not throw any exception.

taylorotwell · 2017-06-18T17:48:39Z

Re-submit this when all of @lucasmichot's feedback is addressed and I'll take a look. Thanks!

mathieutu added 5 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

joshmanders reviewed Jun 16, 2017

View reviewed changes

lucasmichot reviewed Jun 16, 2017

View reviewed changes

tillkruss changed the title ~~Refactoring mix method.~~ [5.4] Refactoring mix method. Jun 16, 2017

lucasmichot reviewed Jun 17, 2017

View reviewed changes

taylorotwell closed this Jun 18, 2017

mathieutu added a commit to mathieutu/framework that referenced this pull request Jun 19, 2017

Update with PR review comments (laravel#19639).

b6e2d33

mathieutu mentioned this pull request Jun 19, 2017

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

Closed

mathieutu mentioned this pull request Jul 6, 2017

[5.5] Mix refactoring #19895

Closed

[5.4] Refactoring mix method. #19639

[5.4] Refactoring mix method. #19639

Conversation

mathieutu commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieutu Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

lucasmichot Jun 17, 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 17, 2017 • edited Loading

Choose a reason for hiding this comment

lucasmichot Jun 17, 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

taylorotwell commented Jun 16, 2017

m1guelpf commented Jun 16, 2017

mathieutu commented Jun 17, 2017

lucasmichot Jun 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot Jun 17, 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

taylorotwell commented Jun 18, 2017

mathieutu Jun 16, 2017 •

edited

Loading

lucasmichot Jun 17, 2017 •

edited

Loading

mathieutu Jun 17, 2017 •

edited

Loading

lucasmichot Jun 17, 2017 •

edited

Loading

lucasmichot Jun 16, 2017 •

edited

Loading

lucasmichot Jun 17, 2017 •

edited

Loading