Created cached entrypoint look up class #12

codayblue · 2018-11-04T20:19:34Z

Created the CacheEntrypointLookup
modified the EntrypointLookup to have a protected method instead of private to easily just extend it
Created the cache compiler pass that reads the file and will save it to the symfony cache with the service construction
Created a config option to enable the cache class and compiler pass
Enabled the compiler pass in the bundle
Updated tests

This will resolve issue #3

codayblue · 2018-11-14T17:12:17Z

@weaverryan is there anything else needed with this PR?

weaverryan · 2018-11-16T14:43:22Z

src/Asset/CacheEntrypointLookup.php

+    protected function getEntriesData(): array
+    {
+        return $this->entrypoints;
+    }


Hmm. Unfortunately, making this protected isn't the best way to make this extension point - though it was the easiest! ;) It forces us to make the method protected... and it also forces us to remove the @final on the parent class (which we would like to not need to do).

I see why you did it - there is actually a decent amount of logic in EntrypointLookup other than just json_decode() on the manifest.json file.

I have a different idea. Sorry for the back-and-forth - this isn't an area that I usually touch in the Symfony core :). Here's my idea:

Let's always cache. Basically, we will move the caching logic into EntrypointLookup and not make it optional

But, we will not use, for example, the cache.system cache or something like that. Instead, there is a system in the Config component of Symfony for this type of caching - an object called the ConfigCacheFactory.

Basically, we use the ConfigCacheFactory to cache the results of parsing the entrypoints.json file. But, built into that system is a mechanism that will automatically recache it if the file modified time of entrypoints.json is updated (in dev only, of course). It means that, in dev, the cache is loaded the first time you access the page, then reloaded whenever the file changes. In prod, we would create a new "cache warmer" so that it's loaded during cache:warmup.

A good example of this system is the translator:

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml#L19-L21

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Translation/Translator.php#L323-L327

Can you try this out? I think it's ultimately the best solution.

Thanks!

I like this solution better. I just did not know about config cache. I will implement this.

weaverryan

NIce work! I left a few comments - and I know of one other thing we'll need: a cache warmer (so the cache file can be created during cache:warmup). For example: https://github.com/symfony/symfony/blob/254f4c8f7f116733fb32cbb872f9184c094630b1/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php

weaverryan · 2018-11-26T14:45:13Z

src/Asset/EntrypointLookup.php

+        }
+
+        if (!isset($options['cache_dir'])) {
+            throw new RuntimeException('Cache directory not set.');


If it's not set, then we basically just need to disable caching. This is needed, at the very least, for backwards compatibility - we can't add a required option/argument to this class without a BC layer. However, we can trigger a deprecation warning so that we can make this option required in the future.

weaverryan · 2018-11-26T14:49:20Z

src/Asset/EntrypointLookup.php

+EOF
+                , var_export(json_decode(file_get_contents($this->entrypointJsonPath), true), true));
+
+                $cache->write($content);


Here, I believe the second argument is Resource - we need 1 resource that points to the entrypoints.json file. This is the key to how cache invalidation works in the dev environment: by passing that here, the cache will know to regenerate if that file is updated.

codayblue · 2018-12-09T05:33:02Z

@weaverryan I have updated the PR. Sorry for the long wait. Also all the tests passed? There appears to be a deprecation warning from PHP unit that caused Travis to fail.

weaverryan · 2018-12-15T17:12:22Z

I'm going to wait for #17 and #21 before this one - especially #21, which may need to add a constructor arg to EntrypointLookup to fix a bug.

Thanks!

weaverryan · 2019-01-20T01:50:00Z

src/Asset/EntrypointLookup.php

+        });
+    }
+
+    public function warmUp($cacheDir)


The warmup should now use the cache service to read and set the cache - no more ConfigCacheFactory stuff

weaverryan · 2019-01-20T01:53:22Z

src/Resources/config/services.xml

@@ -9,6 +9,9 @@

        <service id="webpack_encore.entrypoint_lookup" class="Symfony\WebpackEncoreBundle\Asset\EntrypointLookup">
            <argument /> <!-- entrypoints.json path -->
+            <call method="setCache">
+                <argument key="$cache" type="service" id="webpack_encore.cache" />
+            </call>


One tricky thing is that there are now more than just this one service for this class. Look at the Extension class in this bundle - you’re now able to configure multiple builds and each will have its own service. We need to call setCache on all of these AND make the warmer work for all of them.

- Altered entry point lookup to check if cache has been added and if it has then cache the file - Altered collection to set the cache for every lookup - Altered services file to create a cache service - Updated tests to test caching - Updated cache warming to warm all the builds Github Issue: #3

weaverryan · 2019-01-20T21:03:15Z

src/CacheWarmer/EntrypointCacheWarmer.php

+    {
+        $entryPointCollection = $this->container->get('webpack_encore.entrypoint_lookup_collection');
+
+        if ($entryPointCollection instanceof EntrypointLookupCollection) {


We could simplify if we added a getAll() method to this object. Then we wouldn’t need to inject builds here. We could just foreach over getAll()

The container interface does not have a get all. Is that a method on the class that gets injected?

Yea, I think we should actually inject the EntrypointLookupCollection service here directly - I think there’s no need to inject the container and then fetch it. Then, yes, the getAll() method would be on that class (the collection class) - it will allow us to easily ask for all the EntrypointLookup instances.

The collection class gets an instance of ContainerInterface this does not have a get all. So should I pass the builds to the collection? So it can track them. When I debug through I see it is the ServiceLocator class from the dependence injection component. I dont see a get all method there but I can see how it works and try to extend it to make that possible.

weaverryan · 2019-01-20T21:03:48Z

src/Asset/EntrypointLookupInterface.php

 use Symfony\Contracts\Service\ResetInterface;
 use Symfony\WebpackEncoreBundle\Exception\EntrypointNotFoundException;

-interface EntrypointLookupInterface extends ResetInterface
+interface EntrypointLookupInterface extends ResetInterface, WarmableInterface


What’s the purpose of the WarmableInterface?

This is to make sure the warmup method is there. I can remove the interface and just make a warmup method in this interface. Another thing is I can remove the interface and not have the warm up method here and the cache warmer can just assume the method exists.

I think we should remove it - the warmer knows exactly what instances it’s workijg with and therefore what methods exist. What’s the purpose of this interface normally?

The warmer knows about the collection and this interface. It does not know about the actual instance. I can have it do an instance check and on what is returned from the collection.

weaverryan · 2019-01-20T21:04:58Z

src/CacheWarmer/EntrypointCacheWarmer.php

+                // If the file does not exist then just skip past this entry point.
+                if (!file_exists($fullPath)) {
+                    continue;
+                }


I think this logic shouldn’t be necessary. We can just call warmUp() - that method is already written to not fail if this file is missing

I will remove this.

weaverryan · 2019-01-20T21:09:02Z

src/Resources/config/services.xml

+            <argument key="$directory">%kernel.cache_dir%/encore/</argument>
+        </service>
+
+        <service id="webpack_encore.cache" class="Symfony\Component\Cache\Adapter\PhpArrayAdapter">


I think we’ll need a new config option to opt into this behavior. If that config option is false, I think we calcium’s just remove this service in the Extension class. Then we just need the on-invalid behavior on the setCache calls to be correct so Symfony just skips the call instead of an error

Do you know what you want the config option to be named?

Probably could be cache: true on the “root” level of the bundle config. IMO, no need to make it possible to disable/enable cache independently for each build - someone can ask for that later. Just one global option.

weaverryan · 2019-01-22T15:55:05Z

src/Resources/config/services.xml

@@ -9,6 +9,9 @@

        <service id="webpack_encore.entrypoint_lookup" class="Symfony\WebpackEncoreBundle\Asset\EntrypointLookup">
            <argument /> <!-- entrypoints.json path -->
+            <call method="setCache">
+                <argument key="$cache" type="service" id="webpack_encore.cache" />


Remove the $key here - the whole "binding by named arguments" thing is really something that should only be used in application land. In a bundle, we should avoid this and pass arguments by order - it'll avoid a minor performance impact when rebuilding the container.

codayblue · 2019-02-12T18:45:46Z

@weaverryan Is there anything else needed for this PR? Are we waiting on another PR?

weaverryan · 2019-03-01T16:10:13Z

Thank you @codayblue and @nicolas-grekas - this was a bit more complex than I gave it credit for initially :)

…as-grekas, weaverryan) This PR was merged into the master branch. Discussion ---------- Created cached entrypoint look up class - Created the CacheEntrypointLookup - modified the EntrypointLookup to have a protected method instead of private to easily just extend it - Created the cache compiler pass that reads the file and will save it to the symfony cache with the service construction - Created a config option to enable the cache class and compiler pass - Enabled the compiler pass in the bundle - Updated tests This will resolve issue #3 Commits ------- c165e64 removing extra use statement 614964e Add cache flag 465aae2 Adjusted code to pass tests and updated tests badc55d - 243c87f Cache Entry Point Files

weaverryan reviewed Nov 16, 2018

View reviewed changes

weaverryan reviewed Nov 26, 2018

View reviewed changes

weaverryan reviewed Jan 20, 2019

View reviewed changes

weaverryan reviewed Jan 22, 2019

View reviewed changes

nicolas-grekas and others added 3 commits January 22, 2019 17:23

-

badc55d

Adjusted code to pass tests and updated tests

465aae2

Add cache flag

614964e

removing extra use statement

c165e64

weaverryan merged commit c165e64 into symfony:master Mar 1, 2019

codayblue deleted the 3-cached-entrypoint branch March 1, 2019 16:51

weaverryan mentioned this pull request Mar 1, 2019

Documenting the WebpackEncoreBundle caching option symfony/recipes#541

Merged

Uh oh!

Created cached entrypoint look up class #12

Created cached entrypoint look up class #12

Uh oh!

Conversation

codayblue commented Nov 4, 2018

Uh oh!

codayblue commented Nov 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codayblue commented Dec 9, 2018

Uh oh!

weaverryan commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codayblue commented Feb 12, 2019

Uh oh!

weaverryan commented Mar 1, 2019

Uh oh!

Uh oh!

weaverryan commented Dec 15, 2018 •

edited

Loading