OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.38k stars 1.12k forks source link

Gulp stuff should be generalized, consolidated out to the root src folder, and documented #5450

Closed DaRosenberg closed 9 years ago

DaRosenberg commented 9 years ago

Background

With VS2015 MS is embracing Grunt/Gulp as the automation tool of choice for client-side assets (styles, scripts, images etc). These tools are built on Node.js and are extremely powerful, flexible and versatile. They are commonly used for tasks such as compiling LESS/SCSS, bundling, minification, source map generation, browser vendor prefixing, etc etc.

True to this direction, Web Essentials for 2015 has dropped support for all these tasks, instead instructing developers to use Grunt/Gulp for this.

VS2015 has a new tool called Task Runner Explorer that allows you to execute your Grunt/Gulp based pipeline and integrate it with you VS workflow by binding Gulp tasks to project events (such as open, build, clean). This tool is also available for VS2013. There is also a tool called Grunt Launcher (slightly misnamed as it also supports Gulp) that adds support for installing all required node modules for your pipeline by right-clicking on your Packages.json file and choosing NPM Install Packages. This tool is available for both VS2015 and VS2013.

Mm and what does this have to do with Orchard?

To enable us to use VS2015, Orchard has already started using Gulp to perform these tasks in a few modules:

Why these modules, you ask? Because these modules were already using Web Essentials to perform these tasks. These modules now all have a Packages.json file (that tells NPM which node modules are required) and a Gulpfile.js which contains the automation logic. (You could of course execute Gulpfile.js yourself using standard Node.js from the command line, but the tools mentioned above let you conveniently execute it from within VS instead, and have it be triggered by various VS events.) These module all have slightly different automation pipeline needs, and so the module dependencies and the Gulpfile.js logic differ between these modules.

Problems with current state

I believe we should not continue further on this path.

Having each module contain it's own complete automation pipeline has a number of issues:

  1. There is a massive amount of duplication.
  2. For every module where want to perform one or more of these tasks, we have to reinvent the wheel and create the whole pipeline and install all Node.js packages etc.
  3. Any improvements will have to be manually applied in each module.
  4. Installing the required Node.js modules (and all their dependencies) creates a massive directory structure under node_modules directly in the module directory. This can be .gitignored to prevent it from going into source control, but due to the way Orchard.Web is published for deployment, their presence on disk causes the publishing to fail.
  5. 5446

    Proposed solution

I think the problems mentioned above could be mitigated if we generalize and centralize the concept out into the root src folder. In other words, create one Gulp automation pipeline that does this work for all modules in the code base.

Essentially I think we should:

  1. Create one Packages.json file in the solution root.
  2. Create one Gulpfile.js file in the solution root.
  3. Create and document a convention for modules to follow in terms of how to organize their client-side assets
  4. Configure that one Gulpfile.js to find client-side assets in all modules (according to that convention) and perform these tasks for all of them.
  5. Remove all Gulp and NPM stuff from individual modules.
  6. Obviously have both Packages.json and Gulpfile.js in source control, but .gitignore the node_modules folder.
  7. Hopefully Task Runner Explorer can integrate the same way with a solution-level Gulp pipeline - otherwise we'll have to figure out a way to overcome that.

This would solve all the issues I think.

Ideally the new Orchard.Resources module propesed in #5438 should also be able to utilize this unified pipeline.

Some general questions we should agree on:

If we can agree on a framework for this, I think it would be good to go through all modules and restructure them to participate. Currently there is a lot of inconsistency across the code base, e.g. some modules have minified versions of some assets but not all etc. This would be a great opportunity to clean this up and get the benefits of bundling, minification, source maps etc. across the whole code base.

Which branch?

I am hesitant on where I think this change should go, 1.9.x or dev? I know it sounds huge and some of you will instinctively say "are you crayzee?? dev of course!" but before you make a conclusion, consider:

I am leaning towards kind of a hybrid:

Finally I think it will be crucial to document this stuff. Both for explaining how it works, but also as guidelines for creating new modules that wish to participate in the client-side asset build pipeline. Once the questions have been sorted out and we agree on a general direction, I will gladly take it upon myself to document this.

DaRosenberg commented 9 years ago

OK, here are my 2 cents on the questions posed.

What tasks should the framework perform?

Based on the types of assets that are currently used by modules in Orchard, I believe a good starting point would be to have the following 4 pipelines provided and automated by the framework:

For each of these four pipelines, the framework would provide three tasks:

What is the convention/contract for modules to utilize the framework?

After giving this some thought I feel it would be very difficult to make this entirely conventions-based (particularly, it's hard to imagine a practical way for modules to specify bundling groups for included assets using a purely conventions-based approach).

Instead I propose a very simple JSON format through which a module can provide its desire to utilize the framework, which assets it wants processed, and how to group them into output files. To opt in to the framework, a module (or a theme for that matter) should have a file named Assets.json in its root folder. The Gulp script will scan modules/themes and look for this file and load it.

Here is a sample fictitious Assets.json file (I think most cases would be much simpler than this, but to illustrate the concepts):

[
    {
        "inputs": [
            "Assets/forms.css",
            "Assets/Bootstrap/bootstrap.less",
            "Assets/Bootstrap/theme.less"
        ],
        "output": "Styles/Styles.css"
    },
    {
        "inputs": [
            "Assets/forms-admin.css",
            "Assets/menu.dynamicforms-admin.css"
        ],
        "output": "Styles/Admin.css"
    },
    {
        "inputs": [
            "Assets/LayoutEditor/Directives/Fieldset.js",
            "Assets/LayoutEditor/Directives/Form.js",
            "Assets/LayoutEditor/Models/Fieldset.js",
            "Assets/LayoutEditor/Models/Form.js"
        ],
        "output": "Scripts/Scripts.js"
    }
]

Notes:

bleroy commented 9 years ago

Thanks for writing this. I've had to solve some of the same problems on DecentCMS, because it's also modular, and also uses Grunt/Gulp/Npm (of course it does, it's Node). It's still work in progress, but I'll share some of what I've found.

First, on Gulp vs. Grunt, DecentCMS currently uses Grunt, but I think I'll move to Gulp eventually. I think it would be a bad idea to support both, and if you're going to choose one, Gulp is more modern and trends to become the standard.

While I've had to move some of the tasks to the "solution" level, I still have one package.json in each module (not just for Grunt: modules are real npm packages, so this is needed for other things). There are however top-level tasks that know how to drill down and execute the local tasks for each module. Under /lib and in the top gruntfile.js, you'll find some of the utilities I've written to achieve that.

Because I have both top-level and local tasks, with top-level ones typically calling the local ones, I can run at the level I choose, as needed. For example, I can run all the tests in the solution in one operation, or I can focus on the test task for one specific module if I need to.

The local gruntfile.js that you'll find in any module is typically a very simple boilerplate copy of the module-gruntfile.js that's at the top-level.

The problem of nested npm dependencies is also something I'm experiencing, so I'm very interested to see what kind of solution we come up with in Orchard. I've given it some thought, and my solution will probably involve leveraging some of the work from the npm team to flatten dependencies, coupled with some additional code in my own dependency injection code. But yes, lots of things will move to the top level, like you're suggesting.

Currently, the tasks that I'm handling are install and update (which recursevely npm installs or updates all module dependencies), test (recursively runs test suites in all modules), imagemin. I have local less, and scripts tasks where relevant, so those are not top-level, only local. This may change.

bleroy commented 9 years ago

Another thing I'd suggest is to put that JSON into package.json: package.json is extensible, and I'm not sure it's a good idea to introduce a new file. Grunt & Gulp already use package.json for a number of things, and it seems like a good thing.

bleroy commented 9 years ago

Another comment on checking-in built js and css: currently we offer two ways to install Orchard, the zip pre-compiled package, and the source code. If we want to keep it simple, the pre-built has no tooling, just the binaries, and that includes built css and js. It just works. Then the source code, which already isn't ready to run before you build it, would just have a couple more build steps. There probably is no point in being able to read the results of the TS compilation or the less process directly in the source tree. The only concern I'd have, as I've expressed before about TS, is that we're growing the list of pre-requisites for people who need to build without VS (that includes many continuous integration setups), but as Gulp is becoming more and more mainstream, this doesn't look like such a burden. At least the runtime list of dependencies is still just ASP.NET MVC.

DaRosenberg commented 9 years ago

Thanks for the good input Bertrand!

I think in your case with DecentCMS it makes a lot of sense to retain a packages.json file in each module, given that they are themselves NPM packages. In our case with Orchard, I think it doesn't so much. But I'm interested in what you mean by Gulp already using Packages.json for a number of things - can you elaborate, or perhaps post some links to more info? A quick search on this didn't provide me with anything.

I'll have a look at what you built for drilling down into modules from the top level, as it sounds like you already solved a lot of what I would need to achieve.

Regarding having output assets in version control, I am very tempted to go the way you are suggesting (i.e. not having them in version control) as it just seems cleaner to me, and for me personally it would not be an issue. But I'm concerned that it'll cause trouble for others getting started with the source, specifically:

  1. You would no longer be able to build Orchard outside of Visual Studio (command line or CI builds etc) unless we also hook the Gulp stuff into MSBuild somehow. You have any experience or ideas on how to do that?
  2. Even in Visual Studio there is a manual step required to get it started, i.e. installing the NPM packages. This needs to be done for the Gulp integration tools to ever recognize Gulpfile.js and allow you to execute its tasks. And it's not very discoverable either, you basically have to just know that it needs to be done. If you don't there are no error messages, no nothing, just 404 when you eventually run the site and it tries to load the resources. Any suggestions on this?
MpDzik commented 9 years ago

I would suggest avoiding any solutions which require Visual Studio to build Orchard. I work in a lot of projects which rely on building Orchard from the console (for example on a build server) without VS. Additionally, I sometimes tend to work outside VS (for example in WebStorm or Sublime Text). Therefore, I think that forcing developers to use only VS to work with Orchard is a bad idea.

IntranetFactory commented 9 years ago

I would suggest avoiding any solutions which require to setup additional tools and dependencies. I always prefer to clone a repo, open the solution, hit F5 and it should work. So no additional tools, no nuget packages, no bower, no npm which is not included in the repo. I see nuget/npm/bower as a convenient way to update dependencies when needed. But I really hate being offline and recognizing that a dependency is missing or in the wrong version. So in our own repos we always commit all dependencies as space is cheap.

DaRosenberg commented 9 years ago

Upon further pondering, I'm realizing I sounded the alarm bell for no reason.

You can of course always build outside of Visual Studio, because Gulp executes naturally from the command line. Visual Studio would never be required. We can either:

  1. Preferably, extend the Orchard.proj MSBuild file to install NPM packages and execute the Gulp tasks, OR
  2. If that's not possible for whatever reason, extend the ClickToBuild.cmd and ´ClickToBuildAzurePackage.cmd` batch files to do the same thing.

The only prerequisite would be having Node.js installed.

The issue I mentioned with discoverability inside Visual Studio however still applies. Any input on how to improve that context would be most welcome.

Piedone commented 9 years ago

If we have the assets in the solution as we have now then I don't see why it would matter too much what build support there is: if you develop Orchard itself then you'll need some tools during development, otherwise in your own solution you're free to do whatever you want (and e.g. write plain CSS, or use Grunt/Gulp/whatever to generate it).

And I'd suggest to keep output files (CSS, JS) in the solution for the sake of simplicity. Can't really see any drawbacks: one pain point could be merge conflicts in generated files, but these are easy to resolve and only impact the development of Orchard (and not everyone developing with it). Storage (even if we talk about repositories) is a non-issue.

Long story short: whatever we do, it shouldn't affect people developing on Orchard, only people developing Orchard itself.

sebastienros commented 9 years ago

My concerns: 1- With this solution, whenever a js/css file is touched then the full gulp pipeline would be restarted, right? As the gulp script wouldn't know which file is modified, and Visual Studio would register a filewatcher on every single asset file. 2- Are you sure the file length issue would be fixed by moving the node_npm folder at the top? 3- I would love to see it not integrated by default with visual studio if I don't want to use it, as today it's starting automatically for nothing, making the build really slow and breaking the command line.

I am worried that the change is huge here for what it adds. And right now the command line build is broken in 50% of the cases because of the file length limitation. A conservative change would be to revert the gulp integration to fix the build scripts, and in parallel work on the solution wide design you suggested. An even more conservative strategy would be to do it in 2.0 only.

bleroy commented 9 years ago
  1. I don't think this is true: doesn't VS simply emit an event when you save a file, that the Gulp stuff can hook onto? Remember that that Gulp stuff is already used by pretty much the whole Node community. As far as I know, it works great without ruining perf.
  2. It would be mitigated, but it would still be a problem for modules that have a very deep dependency graph. That is, until the next version of npm, where they will flatten things as much as they can. By then, that concern will be completely gone except maybe in very very marginal cases.
  3. Yes, the watching tasks should be opt-in, but that may be an issue with how the feature is built in VS more than how we use it.
DaRosenberg commented 9 years ago

All that @bleroy said, plus some comments:

  1. No, not true. Only the file that actually changed will be sent through the Gulp pipeline.
  2. For our scenario it would be fixed. The file length issue is only an issue when publishing, and if we move it all to the top level publishing will never see it, as publishing only happens using either Orchard.Web or Orchard.Azure.Web as roots.
  3. We can remove the event bindings if you prefer, and instead you'll need to invoke the "build" and "watch" tasks manually from Task Runner Explorer. However, depending on what you mean by "breaking the command line" I'm pretty sure that issue will go away with this refactoring.

I wouldn't call this change huge, but certainly it's significant. I think you're underestimating the value it will add though. I'm intending to get started on it fairly soon, but in the meantime I guess it wouldn't hurt to remove the event bindings if it's breaking things for you or slowing them down unacceptably.

sebastienros commented 9 years ago

Do I summarize all the goals with these points ?

  1. All assets should be in the solution in the end, even if they are updated by Gulp.
  2. If there are no changes of the source files, the process should not be started automatically by Visual Studio. And the process should not be started by a command line build, meaning that if I haven't changed any assets from Layout, I don't want any package to be downloaded, or all assets to be rebuilt.
  3. Each module contains a Gulp file to build its own assets. If a module asset is changed, only this Gulp file is running.
  4. If downloaded, the packages should not live inside each module by at the root, for instance in the /build folder.
DaRosenberg commented 9 years ago
  1. Correct, output should remain in source control.
  2. Command line build - correct. Running the gulp pipeline should be considered an edit activity, not a build activity. As for starting it from VS, well... my vision was for it to be hooked into the VS events (open, build, clean) but if others think that's too intrusive, we can settle for running it manually.
  3. No, incorrect. There is only one gulp file, at the solution level, but each module contains a small "asset manifest" that is input to the global process. But correct that only changed assets will be processed, not everything at every change. The global gulp file will be smart enough to only trigger processing for items which are newer than their respective outputs.
  4. Correct.

I plan to work on this tomorrow.

DaRosenberg commented 9 years ago

Implemented this today - I'm very happy with the result! It's fast, seamless, clean, unintrusive (you won't notice it unless you choose to edit and recompile client-side assets) and most importantly, it fixes the publishing issues we've been seeing.

Pushed it to 1.9.x as planned, and merged to dev so we can continue to apply it for all the other modules there over time.

The new solution-level Gulpfile.js and Package.json files are located here:

image

In Task Runner Explorer you'll find 3 tasks:

image

As per @sebastienros's request, I did not bind the Gulp tasks to any Visual Studio events. You must manually invoke one of these three tasks from Task Runner Explorer, depending on what you intend to do.

Please try it out and let me know what you think!

Here's an example from Orchard.DynamicForms of what an Assets.json file might look like:

[
    {
        "inputs": [ "Assets/JavaScript/Lib/**/*.js" ],
        "output": "Scripts/Lib.js"
    },
    {
        "inputs": [ "Assets/JavaScript/LayoutEditor/**/*.js" ],
        "output": "Scripts/LayoutEditor.js"
    },
    {
        "inputs": [ "Assets/CSS/*.css" ],
        "output": "Styles/@.css"
    }
]

Note the Styles/@.css syntax in the last asset group. This means basically "don't perform any bundling, create one output file per input file, using this target path where @ is the base file name of the input asset file".

I will proceed to write up some documentation on how the thing works.

Skrypt commented 9 years ago

Works perfectly. I like it. :+1:

sebastienros commented 9 years ago

About the assets file, is it a standard format or a custom one?

DaRosenberg commented 9 years ago

New standard, patent pending. ;)

thekaveman commented 9 years ago

Just in case this isn't obvious to someone else (it wasn't for me)...

When you first load Orchard in VS 2015 and look at the new Task Runner Explorer, you'll see that it has located Gulpfile.js, but is not finding the tasks contained therein:

image

This is because you are missing the required NPM packages. Either use the command line, or install the Grunt Launcher Visual Studio extension, which allows for right-clicking on the Package.json file to install NPM packages. Afterwards, refresh Task Runner Explorer to see the tasks:

image

@DanielStolt I know you've mentioned this in various comments on this thread - I wanted to make it explicit with screenshots for others like me who can't seem to slow down and read the whole thing ;-)

dcinzona commented 9 years ago

That's odd, VS2015 automatically downloaded everything in Package.json for me on startup. I do not have the Grunt Launcher VS extension installed. It was all backed in to VS 2015

thekaveman commented 9 years ago

Odd indeed. I opened and closed VS many times, cleaned and rebuilt the solution...nothing.

dcinzona commented 9 years ago

Is Package.json in the solution when you open it or is it just sitting in the src directory, but not included in the actual VS solution? screen shot 2015-08-05 at 1 06 51 pm

thekaveman commented 9 years ago

Yes, it was included in the solution when I opened.

dcinzona commented 9 years ago

Weird, I wonder if there are some extensions that are breaking it for you? I have this working on two completely separate computers and the Package.json file is automatically parsed successfully on both.

Not only that, but when I make changes to it, the npms are automatically downloaded.

thekaveman commented 9 years ago

I am using the Community edition of VS 2015 - same for you, or do you have a full version?

dcinzona commented 9 years ago

I have VS 2015 Pro... I wonder if that's the issue...

On Aug 5, 2015, at 5:00 PM, Kegan Maher notifications@github.com wrote:

I am using the Community edition of VS 2015 - same for you, or do you have a full version?

— Reply to this email directly or view it on GitHub.

DaRosenberg commented 9 years ago

I have VS 2015 Enterprise and it does not automatically install the packages for me for this particular Package.json file (presumably because it's on the solution level) but it does so for any project level file. However, if I open the file and just save it (no changes) VS installs the packages. No need to install any extensions.

rtpHarry commented 8 years ago

I guess you still have several months before this becomes an issue but I noticed today that Bootstrap 4 is moving from Less to SASS as its preprocessor. At the moment the pipeline doesn't have any support for it.

Seeing as assets.json is going to be promoted as the new standardised way for module and theme developers I think SASS support should probably be integrated into the process even if there aren't any official Orchard modules currently using that part of the pipeline.

DaRosenberg commented 8 years ago

@rtpHarry I do agree. As mentioned in today's meeting, it was always my intention to have the assets pipeline to support both.

mwpowellhtx commented 6 years ago

This may have been addressed, I don't know. I have some boilerplate Gulpfile.js stuff, basically some tasks that apply across a suite of projects in my VS2015 solution. However, there are a couple of minor differences that I want to capture via project level configuration. However, it seems that the VS2015 tooling does not "see" the linked Gulpfile.js (i.e. I literally added the boilerplate as a link, with require for the configuration). Sorry is this is slightly tangential or off topic, but it seems relevant to the proposed question.