hearsayit / HearsayRequireJSBundle

RequireJS integration for Symfony2.
130 stars 55 forks source link

2.0 does not support build_profile option in bundle configuration #43

Closed benglass closed 10 years ago

benglass commented 11 years ago

The current 2.0 branch does not support the build_profile configuration option to allow for external build profile. I found this to be a key feature for any dynamic/runtime generated modules that I needed to mark as :empty so they would not create an error during build because they cannot be located. This is also a key feature for advanced configuration.

ihortymoshenko commented 11 years ago

@afishnamedsquish, yes, you're right. I've removed the build_profile configuration option in the 2.0 version. The build profile's file is generated on the fly now. I made this decision because we had an issue with modules' names which contain dots and the old code was hard to maintain.

I'm not sure that I correctly understand you about "dynamic/runtime generated modules". Can you be more specific? If you can provide examples, it will be great! Thanks!

benglass commented 11 years ago

@IgorTimoshenko Sometimes I will define a module of configuration options which are dynamic within a view. I will then call define('some_options', { dynamic: option, dynamic2: option2 }); in the view itself. This is then loaded later via require('some_options') in the actual module where its used.

some_options must be excluded from the build profile (or marked as empty:) or an error is generated.

I did not have luck using the excludes yml option in terms of telling the build profile to ignore the module. I had the same issue with the fos js routing bundle where I exposed the router by including a define call in my main module that refers to the global variable Routing (since this from a non ASM file in a different bundle). I also needed to make sure the the router was ignored by the build profile since its provided at run-time. There is probably a better way to do this in this case.

The solution I ended up with is switching back to 2.0 branch of this bundle and setting up paths as follows:

hearsay_require_js:
    paths:
        router:
            location: 'empty:'
            external: true

It seems I should be able to use the excludes config option but I could not get it to work.

ihortymoshenko commented 11 years ago

@afishnamedsquish, can this issue be resolved with supporting the config? As for the excludes configuration option, you can look this test. Looks like it works.

benglass commented 11 years ago

@IgorTimoshenko possibly can be resolved via config option, thanks for pointing me in that direction. I'll take another look at the excludes issue and see if I can isolate the problem I was having.

benglass commented 11 years ago

@IgorTimoshenko I'm not sure that the mainConfigFile in combination with the dynamically generated build profile will work for what I am trying to do.

Running a multi-page site, I want to optimize my main module separately from the per page modules. The main module would have jquery and any other globally used modules in it. These modules may be a dependency in the per page modules but they should not be optimized in because then they would be loaded twice.

I need a way to specify for my modules which modules are included/excluded. This can normally be achieved using a build profile although it appears you can do something similar with a mainConfigFile. However it appears build profile settings will override this. In any case, even trying to set up excludes for my modules using mainConfigFile results in an error during assetic:dump due to incompatibility with the way the build_profile is constructed.

Example Multi Page Build Profile

Error: Error: If the "modules" option is used, then there should be a "dir" option set and "out" should not be used since "out" is only for single file optimization output.

My main config file looks like this (its definitely being used as it allows for the definition of dynamic modules using the empty: syntax, which I previously had to do by setting location to empty: and external to true in config.yml)

requirejs.config({
    paths: {
        router: 'empty:'
    },
    modules: [
        {
            name: "main"
        },
        {
            name: "password-profile",
            exclude: ["config"]
        }
    ]
});
psrpinto commented 11 years ago

@afishnamedsquish I had the same requirement and "fixed it" by adding a modules section to the config (I'm running my own fork):

modules:
    -
        name: common
        deps:
            - jquery
            - underscore
    -
        name: views/home
        exclude: [common]

See this commit. If there's interest I can turn this into a PR. @IgorTimoshenko

benglass commented 11 years ago

@regularjack thanks this would be super helpful. @IgorTimoshenko I'm not sure if you have a recommendation on an alternate approach to this but being able to do per module exclusions seems to me a requirement for multi-page websites so this seems like it would be a great feature to support.

ihortymoshenko commented 11 years ago

@regularjack, looks interesting.

I can make changes which corresponds to your needs. I think there is no sense in a PR as I've found a few inaccuracies in the commit, and as a result we will spend more time on code review and fixes.

Thanks for the contribution, guys!

psrpinto commented 11 years ago

Yeah, I realize that commit is not PR-ready, it was a quick hack. I'm looking forward to having this feature "upstream" so that I can stop using my fork.

benglass commented 11 years ago

@IgorTimoshenko let me know if there is anything in this issue or anywhere else where you could use contributors. I'd like to use 2.0 in production and the multi-module optimization with exclusions is key to my use cases so I'd be happy to contribute to making that a reality if you have a preferred approach to the one @regularjack implemented.

ihortymoshenko commented 11 years ago

@benglass, I've tried to implement this in the master branch and I've caught the following error message:

  [Assetic\Exception\FilterException]                                                                          
  An error occurred while running:                                                                             
  '/usr/bin/node' '/vagrant/symfony/app/../r.js' '-o' '/tmp/build_profilep04AFc.js'                            

  Output:                                                                                                      
  Error: Error: If the "modules" option is used, then there should be a "dir" option set and "out" should not  
   be used since "out" is only for single file optimization output.                                            
      at Function.build.createConfig (/vagrant/symfony/r.js:24922:19)                                          

  Input:                                                                                                       
  require([                                                                                                    
      'demo/modules/person/collection'                                                                         
  ],function(collection){                                                                                      
      console.log(collection);                                                                                 
  });

The config is:

modules:
  -
    name: a
    exclude: [b]
  -
    name: b
    exclude: [a]

So I'm not sure that is possible to implement now. I can propose to consider work with the exclude configuration option. If this configuration option doesn't work as expected, I'll fix it. Just let me know.

I also have some thoughts after reading the changes in the commit. What do you think about providing the ability to specify configuration for each bundle separately?

benglass commented 11 years ago

@IgorTimoshenko I also ran into the same errors before in trying to define excludes for my modules individually. It seems that the current format of the build profile being generated by this bundle is not compatible with specifying individual module settings.

I think the multi-module config could still work with the way you have the build profile implemented but you would need to set the excludes option (as you suggested) and populate it with the excludes ONLY for the module that is being optimized. I'm not sure how you would determine which module is being optimized. Perhaps in the assetic filter if you know the original name of the optimized file you can compare that against the paths to find the module name. Then you can look in the bundle config to see if a config is specified for that module and add it to the excludes for the build profile being generated.

The global excludes option is not helpful for multi-module optimization because module exclusion differs on a per-module basis.

I am not sure how being able to specify the configuration separately for each bundle would solve the issue either as you would then have to put your javascript files into separate bundles just to achieve per module settings.

One possible solution would be to allow for the user to specify the build profile as a config option and perhaps just have this bundle parse it and replace bundle syntax such as 'MyBundle:foo' with the correct path. This would allow users to take advantage of any requirejs settings they want. I am not sure what other complications this might cause and how hard this change would be to make but my experience with requirejs and the optimizer is that it can be difficult to get it to do what you want and understand configuration. Combine that with trying to understand how this bundle will interact with the optimizer adds another layer of complexity that makes it difficult to achieve the desired configuration so allowing users to work directly with requirejs via the build profile seems like a good approach that sidesteps this issue. Not sure if that is helpful.

Thanks for continuing to look into this issue and please let me know if i can help any further or with any possible refactorings.

ihortymoshenko commented 11 years ago

@benglass, sorry for the delay in response.

I've thought to allow to specify the build profile's file, parse it, and replace paths. However, it won't work proper as we cannot parse expressions such as function, so I don't want to do it. Moreover, I don't understand how with the ability to specify the build profile's file we can fix the issue with excluding modules as even in this approach we need to know module names in order to apply needed configuration. Perhaps, I'm missing something.

In the end, I think that the changes from @regularjack have the right to live. As I understood, it works fine for him. I've just wanted to add this capability in accordance with the r.js optimizer without any hidden logic.

If we cannot find the easiest way to do it, approach from @regularjack can be applied.

benglass commented 11 years ago

@IgorTimoshenko I think you may be correct that adding the ability to specify a build profile would not solve the issue at handle and parsing it would add significant complexity.

I will try switching to commit that @regularjack indicated and see if it works for my multi page site use case and let you know if I run into any issues.

I don't have a specific alternate suggestion on how to implement the module exclusion.

andersaloof commented 11 years ago

@benglass I forked the bundle today since I needed to specify multiple output modules/files for a project, and I wasn't allowed due to this error.

Output:                                                                                                      
  Error: Error: If the "modules" option is used, then there should be a "dir" option set and "out" should not  
   be used since "out" is only for single file optimization output.   

My fork and commit is here https://github.com/andersaloof/HearsayRequireJSBundle/commit/a3f4fde0186a6557dded8a5d8b8497dc68d14aec

My config btw is looking like this

hearsay_require_js:
    base_dir: %kernel.root_dir%/../common/Resources/js
    paths:
      Default: %kernel.root_dir%/../src/VW/CampaignBundle/Resources/js/Default
      sommer2013: %kernel.root_dir%/../src/VW/CampaignBundle/Resources/js/sommer2013
    optimizer:
      path: %kernel.root_dir%/../common/Resources/js/r.js
      hide_unoptimized_assets: true
      options:
        modules:
          - name: Default/Main
          - name: sommer2013/Main

Dunno if this fixes anything for you, but it made it possible for me to output several different built js files, and the exclude/include options etc from a config.yml per module should be kept.

heldr commented 10 years ago

Hi,

I've just wanted to add this capability in accordance with the r.js optimizer without any hidden logic.

@IgorTimoshenko did you find an approach instead of using a forked version?

psrpinto commented 10 years ago

I've been using my fork in production for several months without any issues. Just to make it clear, my solution allows defining per-module exclusions @benglass

I can turn this into a PR and we can continue the discussion there. What do you say @IgorTimoshenko ?

ihortymoshenko commented 10 years ago

@regularjack, it sounds reasonable, let's try. Don't forget to write tests and update docs ;)

fduch commented 10 years ago

@regularjack, i've took your idea for modules optimizations and built this solution on the top of current version of the Bundle, because we needed to optimize multipage application and we are working with newer version of the bundle (your fork was born before @IgorTimoshenko completely rewrite RJsFilter).

Besides i've added support of include attribute in module optimization section (it refers to the r.js possibility explicitly include some module into the compiled version of another one). I've also made some speed optimizations, added some tests and updated docs.

@regularjack, @IgorTimoshenko if you do not mind, i can suggest my fork as a pull request. What do you think?

ihortymoshenko commented 10 years ago

@fduch, I don't mind.

psrpinto commented 10 years ago

@fduch Sounds great, I actually don't have much time available for this at the moment. Thanks!

fduch commented 10 years ago

Ok, https://github.com/hearsayit/HearsayRequireJSBundle/pull/58

jifeon commented 10 years ago

This is excellent, guys! Thanks you