mariusbalcytis / webpack-bundle

Bundle to Integrate Webpack into Symfony
MIT License
122 stars 36 forks source link

Custom node_modules location, working directory, environment variables #38

Closed steven-pribilinskiy closed 7 years ago

steven-pribilinskiy commented 7 years ago

In our environment we have multiple kernels under app directory e.g. symfony-project/app/[kernel]/config/ and the location of node_modules is shared among these. The cache and Resources directories location also specific, e.g. symfony-project/user/app/[kernel]/cache/, and it is the %kernel.root_dir% so node_modules is not even in ascendant directory relatively to config.yml and webpack.config.js. Probably this bundle was meant to be used in simple environments, hence several issues arose.

  1. The 'app' => '%kernel.root_dir%/Resources/assets', is hardcoded and leads to an error. Must be overridden.

  2. When webpack process is started in %kernel.root_dir%/.. it won't find node_modules directory and webpack-dashboard is obviously fails to run.

tty can be disabled by providing an empty array or alternatively a direct path in the actual node_modules can be specified

Usually it is possible to use export NODE_PATH=... before calling a node script but looks like such possibility, to add custom environment variables, is not provided in bundle's WebpackCompiler class (btw, ProcessBuilder has an addEnvironmentVariables method that can be used instead of command line concatenation in addEnvironment).

Another webpack integration bundle, the hostnet/webpack-bundle provides the node_modules_path key in config, though we would really like to stick with maba's implementation.

The maba_webpack.root_directory parameter in services.xml have a hardcoded %kernel.root_dir%/.. value and is used in WebpackCompiler to set the working directory and as noted in that xml it is Not configurable from config.yml. However it maybe solve the problem. Please reconsider making it configurable. Thanks

steven-pribilinskiy commented 7 years ago

@mariusbalcytis I may try to implement these features in a PR

mariusbalcytis commented 7 years ago

About first problem - I think overriding alias is the way ahead. Or do you think behavior should be changed somehow?

node_modules could be fixed by changing configuration for executables and TTY prefix, but it's really more clearer to add configuration for root directory as you've suggested.

Could you overwrite maba_webpack.root_directory parameter in your config.yml or parameters.yml and check if this would resolve your issue? In that case I'd extend configuration for it.

As I understand, you also need to change node_modules location and/or environment? Would this still be needed? If environment variables would be changed, reconfiguration of executables would still be needed as they include path from working directory (if it's not by current convention in your case).

Pull requests are also welcome - just try to maintain the code style and don't break the tests ;)

By the way, export was added as setEnv was not working as expected, but as I see now, ProcessBuilder can inherit current environment which should resolve the initial issue. Thanks for noticing, I'll check that out.

steven-pribilinskiy commented 7 years ago

imho the @app alias is excessive and it brings troubles to those who don't have Resources directly in the app dir. It's easy for a dev to add this if required, based on another hardcoded alias, the @root.

Yep, my current webpack.yml that is imported to config.yml looks like this (I had to put it to a separate file because it became unexpectedly large):

maba_webpack:

    # webpack.config.js must be under a sibling directory of node_modules
    # https://github.com/mariusbalcytis/webpack-bundle/issues/38
    config:
        path: '%dir.symfony%/webpack/symfony/symfony-webpack.config.js'
        parameters:
            logLevel: '%webpack_log_level%'

    bin:
        webpack:
            executable:
                - node
                - '--max-old-space-size=4096' # 4GB
                - '%node_modules%/.bin/webpack'

        dev_server:
            executable:
                - node
                - '--max-old-space-size=4096' # 4GB
                - '%node_modules%/.bin/webpack-dev-server'

            # currently webpack-dashboard cannot be optional (toggled with a boolean through parameters.ini)
            # https://github.com/mariusbalcytis/webpack-bundle/issues/39
            tty_prefix:
                - node
                - '%node_modules%/webpack-dashboard/bin/webpack-dashboard.js'
                - '--'

            arguments:
                - --inline
                - --host
                - '%webpack_host_ip%'

    asset_providers:
        -
            type: twig_bundles
            resource:
                # Backend
                - OpenbizboxBackendBundle
                - OpenbizboxDesignWizardBundle
                - OpenbizboxSetupBundle
                # Frontend
                - OpenbizboxCheckoutBundle
                - OpenbizboxFrontendBundle

    aliases:

        additional:
            app: '%dir.user%/app'
            # Backend vendor
            be_node_modules: '%node_modules.backend%'
            be_bower: '%node_modules.backend%/@company/backend-vendor/bower_components'
            be_vendor: '%node_modules.backend%/@company/backend-vendor/vendor'
            # Frontend vendor
            node_modules: '%node_modules.frontend%'
            bower: '%node_modules.frontend%/@company/frontend-vendor/bower_components'
            vendor: '%node_modules.frontend%/@company/frontend-vendor/vendor'

        register_bundles:
                # Backend
                - OpenbizboxBackendBundle
                - OpenbizboxDesignWizardBundle
                - OpenbizboxSetupBundle
                # Frontend
                - OpenbizboxCheckoutBundle
                - OpenbizboxFrontendBundle

Not too complicated, simply two kernels - for the frontend and the admin area.

As you can see, in order to be able to use your bundle currently paths to all scripts had to be overridden.


Could you overwrite maba_webpack.root_directory parameter in your config.yml or parameters.yml and check if this would resolve your issue? In that case I'd extend configuration for it.

root_directory cannot be overwritten

 Unrecognized option "root_directory" under "maba_webpack"

that's because $ignoreExtraKeys by default is false for ArrayNodeDefinition of a Symfony\Component\Config\Definition\Builder. In order to not throw an error, when extra keys should just be ignored without an exception, the ignoreExtraKeys(false) should be called on $rootNode (maba_webpack) in Configuration::getConfigTreeBuilder. First argument is to not remove extra keys.

But that is not enough, because the final parameters that are merged to Symfony's DependencyInjection\ParameterBag are added in MabaWebpackExtension the loader of bundle's config and here the params of main app's config.yml (that are provided to load function in $configs array) are selectively applied with $container->setParameter.


Pull requests are also welcome - just try to maintain the code style and don't break the tests ;)

Regarding code style, I prefer chained (fluent) API, e.g. instead of:

        $prototype = $assetProviders->prototype('array');
        $prototypeChildren = $prototype->children();
        $prototypeChildren->scalarNode('type');
        $prototypeChildren->variableNode('resource');

I would write:

        $assetProviders->prototype('array')
                $prototype->children()
                        ->scalarNode('type');
                        ->variableNode('resource')
                ->end()
        ;

it's also a preferred style in Symfony docs.

Also I prefer yml over xml for configs and would split services.xml to parameters.yml and services.yml.

If you don't mind I would make such small changes, in a separate pull request if you wish.


As I understand, you also need to change node_modules location and/or environment?

Looks like you already had plans to make a configurable prefix. Currently an empty array is provided as $prefix to buildProcess() in compile and exec in compileAndWatch on Linux systems.


By the way, export was added as setEnv was not working as expected

It still works unexpected when node_modules directory is in a different place :) and also limits possibilities of command-line customization, the prefix part to be precise.

The $processBuilder->setPrefix($ttyPrefix); is

The NODE_PATH is required for require-d scripts but not for the called scripts, i.e. calling 'node' 'node_modules/webpack/bin/webpack.js' will not work without proper working directory. So really is a cd %node_modules% required before the default maba_webpack.bin.webpack.executable. And here we have a problem. E.g. with following configuration

maba_webpack:
    bin:
        webpack:
            tty_prefix:
                - cd
                - '%node_modules%;'

we get an error

[Symfony\Component\Process\Exception\ProcessFailedException]
  The command "TTY_MODE=on 'cd' '/project/user/app/backend/../../../core/symfony/node_modules;' 'node' '
  node_modules/webpack/bin/webpack.js' '--config' '/obb/openbizbox/user/app/backend/cache/dev/webpack.config.js
  '" failed.
  Exit Code: 1(General error)
  Working directory: /project/user/app/backend/..
steven-pribilinskiy commented 7 years ago

Today I had problems with the hardcoded manifest_file_path too, again, because of our multi-kernel approach. To avoid running multiple webpack dev-servers for every bundle (increases complexity) we need to have a single location for cache files. Every kernel have it's own console command, but the list of commands is same, thus e.g. when running app/kernel1/console the %kernel.cache_dir% is user/app/kernel1/cache and you can imagine where the cache_dir points for another kernel.

Having ability to override the %kernel.cache_dir% will give ability to use webpack in multi-kernel environments like ours.