Allow to define multiple entrypoints and load assets from each of them. #17

shulard · 2018-11-15T22:40:16Z

This PR is added to fix the issue #13. With that new code, you can configure your project like this :

webpack_encore:
    # the "default" build
    output_path: '%kernel.project_dir%/public/build'
    builds:
        frontend: '%kernel.project_dir%/public/frontend/build'

Then in your Twig template :

{{ encore_entry_script_tags('entry1', 'frontend') }}

weaverryan

Hey @shulard!

Wow! Great job with this big feature! I've left a series of comments - it's all just architecture related - avoiding BC breaks, making things lazy for performance, etc. Let's get this guy finished - I think a lot of people are excited for it :).

Cheers!

weaverryan · 2018-11-16T14:50:41Z

src/Asset/TagRenderer.php

+    public function __construct(
+        EntrypointLookupInterface $entrypointLookup,
+        EntrypointLookupCollection $entrypointLookupCollection = null,
+        Packages $packages


This reordering of the args breaks BC. What if we did this:

Replace the first arg entirely with EntrypointLookupCollection

But, don't add the type-hint for EntrypointLookupCollection (as that would be a BC break). Instead, we would do an instanceof check in the constructor: if the user is passing an EntrypointLookupInterface, we would trigger a BC warning (example: https://github.com/symfony/symfony/blob/63d34515a51819a6a6b3a94d598e8e37068e85ce/src/Symfony/Component/HttpKernel/DataCollector/ConfigDataCollector.php#L36). In this situation, we would instantiate a new EntrypountLookupCollection() and pass in just the one EntrypointLookupInterface object. If they pass something that is neither of these 2 types, we would throw a TypeError

Ok, I understand the point about BC breaks 😄

Only one question about the exception message. For the TypeError, I think:

The "$entrypointLookupCollection" argument must be an instance of EntrypointLookupCollection.

For the BC warning :

The "$entrypointLookupCollection" argument in method "__construct()" must be an instance of EntrypointLookupCollection.

We do you think ?

weaverryan · 2018-11-16T14:53:12Z

src/DependencyInjection/WebpackEncoreExtension.php

+        $container->getDefinition('webpack_encore.entrypoint_lookup_collection')
+            ->replaceArgument(0, array_map(function ($item) {
+                return $item.'/entrypoints.json';
+            }, $config['builds'] ?? []));


Let's pass in the "default" path here as well - we could give it a key like default (and add some validation in the Configuration class to make sure there is no key called default set under builds

Also, is it possible for $config['builds'] to not be set? Shouldn't it always be set thanks to the Configuration class (and so the $config['builds'] ?? [] is unnecessary?)

I've updated that part to check for default build name and I removed the ?? operator.

weaverryan · 2018-11-16T14:54:31Z

src/Asset/EntrypointLookupCollection.php

+    public function getEntrypointLookup(string $key): EntrypointLookupInterface
+    {
+        if (!isset($this->buildEntryPoints[$key])) {
+            throw new UndefinedEntrypointException(sprintf('Given entry point "%s" is not configured', $key));


I think you're mixing up "entrypoint" and "builds" in a few places. This should be something more like UndefinedBuildException, right? And, in TagRenderer, the argument to private function getEntrypointLookup(string $entrypointName = null) should really be $buildName, right?

Yes you're right, I update.

weaverryan · 2018-11-16T14:55:01Z

src/Asset/TagRenderer.php

+    public function renderWebpackScriptTags(
+        string $entryName,
+        string $entrypointName = null,
+        string $packageName = null


Unfortunately, args 2 and 3 need to be reversed, otherwise it's a BC break.

And put the method back on one line (I sometimes do this in my code, but we don't do this in Symfony)

I've reordered the arguments.

weaverryan · 2018-11-16T14:57:49Z

src/Twig/EntryFilesTwigExtension.php

+        string $entryName,
+        string $entrypointName = null,
+        string $packageName = null
+    ): string {


Same as above: the 2nd and 3rd args need to be rearranged, unfortunately. I don't think there's a good way to "reorder" these args without breaking BC, except adding an entirely new Twig function.

I've reordered the arguments.

weaverryan · 2018-11-16T14:59:04Z

src/Twig/EntryFilesTwigExtension.php

    }

-    private function getEntrypointLookup(): EntrypointLookupInterface
+    private function getEntrypointLookup(string $entrypointName = null): EntrypointLookupInterface


?string $entrypointName

Let's allow the value to be null, but force it to be passed to this function. Same in TagRenderer

Since entrypoint name is now always a string (default value is _default), I think allowing null is not useful anymore.

weaverryan · 2018-11-16T14:59:26Z

src/Twig/EntryFilesTwigExtension.php

+            return $this->container->get('webpack_encore.entrypoint_lookup');
+        }
+        return $this->container->get('webpack_encore.entrypoint_lookup_collection')
+            ->getEntrypointLookup($entrypointName);


Once we make the EntrypointCollection also able to handle the "default" build, this should simplify

Yes, I've removed the lookup / collection usage to rely only on collection.

weaverryan · 2018-11-16T15:03:01Z

src/Asset/EntrypointLookupCollection.php

+    {
+        $this->buildEntryPoints = array_map(function ($path) {
+            return new EntrypointLookup($path);
+        }, $buildEntryPoints);


Hmm. Let's tweak this slightly:

Make this class receive a ContainerInterface, which will be a service locator (like we use in the Twig extension) containing all of the EntrypointLookup objects.

In WebpackEncoreExtension, for each "build", register a new service dynamically. Then, create a container/service locator for all of the builds, and pass that here.

That will allow all EntrypointLookup objects to be managed by the container, but also passed lazily to this class, so that we're not instantiating ALL of the EntrypointLookup objects just to use one of them.

I've just pushed the changes.

weaverryan · 2018-11-25T18:13:32Z

Ping @shulard! Are you able to make the changes I suggested?

shulard · 2018-11-26T09:51:29Z

Hello @weaverryan, sorry about the delay but the last week was really busy. I'm able to perform requested changes I hope this will be done today.

shulard · 2018-11-26T18:29:28Z

Hello @weaverryan I've updated all the requested changes (I think 😄).

For me the latest point is the exception messages that maybe need to be refined.

frederickhamla · 2018-12-04T15:15:02Z

Hi !

@weaverryan : Thanks for your work, happy for this feature. Did you know when you merge this request ?

@shulard : nice job !

weaverryan

This is awesome! Sorry for the delay - only minor comments!

weaverryan · 2018-12-15T17:07:16Z

src/DependencyInjection/Configuration.php

+                    ->validate()
+                        ->always(function ($values) {
+                            if (isset($values['_default'])) {
+                                throw new \Symfony\Component\Config\Definition\Exception\InvalidDefinitionException("Key '_default' can't be used as build name.");


Minor - add a use statement for this

Ok, updated.

weaverryan · 2018-12-15T17:09:51Z

tests/IntegrationTest.php

+        $this->assertNotContains(
+            '<link rel="stylesheet" href="/build/styles4.css">',
+            $html2
+        );


Hmm, could we split this into a separate test instead of adding to this test? It makes it a bit harder to understand.

I've created two test cases: one with contains check and one with "not contains check".

weaverryan · 2018-12-15T17:10:23Z

tests/IntegrationTest.php

@@ -75,6 +103,9 @@ public function registerContainerConfiguration(LoaderInterface $loader)

            $container->loadFromExtension('webpack_encore', [
                'output_path' => __DIR__.'/fixtures/build',
+                'builds' => [
+                    'other' =>  __DIR__.'/fixtures/other-build'


Instead of other - how about different_build. The problem is that there is an other_entry in the main build, so it's confusing.

Ok, I've renamed it.

This collection will be used to store all the different builds EntrypointLookup instances.

Add a new parameter to define in which entrypoint the assets must be found. Also implement a fallaback system: If no named entrypoint is given, use the default one.

weaverryan · 2018-12-20T14:34:48Z

Thank you for your great work Stéphane!

… each of them. (shulard) This PR was squashed before being merged into the master branch (closes #17). Discussion ---------- Allow to define multiple entrypoints and load assets from each of them. This PR is added to fix the issue #13. With that new code, you can configure your project like this : ```yaml webpack_encore: # the "default" build output_path: '%kernel.public_dir%/build' builds: frontend: '%kernel.public_dir%/frontend/build' ``` Then in your Twig template : ```twig {{ encore_entry_script_tags('entry1', 'frontend') }} ``` Commits ------- 0f3134b Add test regarding EntrypointLookupCollection. a238fe7 Handle the new Entrypoint collection to render paths. 2c38514 Create a new exception when the Entrypoint is not defined. 59bbd29 Update extension to load entrypoint build path. 0f69516 Add an EntrypointLookupCollection. ebe61d9 Allow to define a list of entrypoints to be used in the project.

seyfer · 2019-01-06T14:20:11Z

Great PR. 👍
Docs need to be updated too and show how multiple entry points can be used.

Also would be great to have v1.0.1 or v1.1.0 release tag with this feature.

seyfer · 2019-01-14T11:46:50Z

@weaverryan Does it break dev-server --hot?
See my comment here symfony/webpack-encore#478 (comment)

nyroDev · 2019-01-15T09:21:06Z

I know this have been merged but not yet published, but I found an inconsistency between entrypoints and manifest configuration.

In assets, the configuration might looks like:

framework:
    assets:
        json_manifest_path: '%kernel.project_dir%/public/build/manifest.json'
        packages:
            mobile:
                json_manifest_path: "%kernel.project_dir%/public/buildMobile/manifest.json"

And in webpack_encore, with this PR, this might look like :

webpack_encore:
    # The path where Encore is building the assets.
    # This should match Encore.setOutputPath() in webpack.config.js.
    output_path: '%kernel.project_dir%/public/build'
    builds:
        mobile: '%kernel.project_dir%/public/buildMobile'

My first suggestion would be to simply rename builds into packages directly. (I could PR it if needed)

And then, maybe we can have a more global thinking later in order to merge both configuration and to have only 1 place to configure the output folder.
One solution might be that webpack_encore bundle read the json_manifest_path and deduce the entrypoints path from it.

shulard · 2019-01-15T09:30:02Z

@nyroDev, for me this is two different things. Your first code sample is related to the Asset component configuration. package mean asset package here so you can define multiple asset stores to be used in your application.

Your second sample is the webpack_encore bundle configuration. In that context, the build term mean a specific webpack build.

For me those two terms are relevant in their specific context. Event with that PR merged, you can use different asset packages with different webpack builds. Each of these features allow to control specific part of the asset management in your app.

ureimers · 2019-01-22T15:28:46Z

Hi @shulard,
you're using the parameter %kernel.public_dir% but that one's not defined in Symfony. At least not in Symfony 4.2:
You have requested a non-existent parameter "kernel.public_dir". It's not hard, to change that to '%kernel.project_dir%/public' but I'm just curious.

Am I missing something?

shulard · 2019-01-28T19:44:49Z

Hi @ureimers,

When are you getting that error ? I don't find any usage of public_dir in the code, only in the README which maybe contains a bad example...

ureimers · 2019-01-28T20:41:23Z

@shulard, no you're right, I just took it from the README (and your initial post from this PR) and thought that maybe my SF 4.2 was incorrectly setup. It was just out of curiosity.

Cheers

shulard · 2019-01-29T07:48:36Z

Ok 😄

It's an important feedback if the variable doesn't exists, I'll make a tiny PR to update the README.

shulard · 2019-01-29T09:35:21Z

@ureimers, I've created a tiny PR to update the README and I've updated that PR description to avoid confusion 😉. Thank you for the feedback !

ureimers · 2019-01-29T09:39:00Z

@shulard, you're welcome and thank you for the quick response!

…d. (shulard) This PR was merged into the master branch. Discussion ---------- Remove unknown variable `%kernel.public_dir%` from README.md. As said in #17 (comment), this variable does not exists in Symfony 4.2 and must not be used in the README as a valid example. Commits ------- 9996437 Remove unknown variable `%kernel.public_dir%`

weaverryan requested changes Nov 16, 2018

View reviewed changes

shulard force-pushed the feature/multiple-entry-points branch 2 times, most recently from 161a5e8 to 0ddfb18 Compare November 26, 2018 18:27

weaverryan requested changes Dec 15, 2018

View reviewed changes

weaverryan mentioned this pull request Dec 15, 2018

Created cached entrypoint look up class #12

Merged

shulard added 6 commits December 19, 2018 11:23

Allow to define a list of entrypoints to be used in the project.

ebe61d9

Add an EntrypointLookupCollection.

0f69516

This collection will be used to store all the different builds EntrypointLookup instances.

Update extension to load entrypoint build path.

59bbd29

Create a new exception when the Entrypoint is not defined.

2c38514

Handle the new Entrypoint collection to render paths.

a238fe7

Add a new parameter to define in which entrypoint the assets must be found. Also implement a fallaback system: If no named entrypoint is given, use the default one.

Add test regarding EntrypointLookupCollection.

0f3134b

shulard force-pushed the feature/multiple-entry-points branch from 0ddfb18 to 0f3134b Compare December 19, 2018 10:23

weaverryan merged commit 0f3134b into symfony:master Dec 20, 2018

shulard mentioned this pull request Jan 9, 2019

Is there a way to work with multiple entrypoints ? #13

Closed

This was referenced Jan 11, 2019

problem with Multiple Webpack Configurations symfony/webpack-encore#477

Closed

Support for the entrypoints.json with multiple webpack config symfony/webpack-encore#466

Closed

This was referenced Jan 17, 2019

Catch error with entrypointlookups in sub-requests #21

Closed

Work with multiple Encore configurations #30

Closed

shulard mentioned this pull request Jan 29, 2019

Remove unknown variable %kernel.public_dir% from README.md. #38

Merged

plashenkov mentioned this pull request May 16, 2020

Multiple entrypoints, manifest.json, entrypoints.json and encore_entry_script_tags() #92

Closed

Uh oh!

Allow to define multiple entrypoints and load assets from each of them. #17

Allow to define multiple entrypoints and load assets from each of them. #17

Uh oh!

Conversation

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

Uh oh!

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!

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!

weaverryan commented Nov 25, 2018

Uh oh!

shulard commented Nov 26, 2018

Uh oh!

shulard commented Nov 26, 2018

Uh oh!

frederickhamla commented Dec 4, 2018

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!

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!

weaverryan commented Dec 20, 2018

Uh oh!

seyfer commented Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seyfer commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyroDev commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shulard commented Jan 15, 2019

Uh oh!

ureimers commented Jan 22, 2019

shulard commented Nov 15, 2018 •

edited

Loading

seyfer commented Jan 6, 2019 •

edited

Loading

seyfer commented Jan 14, 2019 •

edited

Loading

nyroDev commented Jan 15, 2019 •

edited

Loading