OrchardCMS / Orchard

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

Gulpfile.js doesn't support custom module and theme folders #6822

Open rtpHarry opened 8 years ago

rtpHarry commented 8 years ago

Custom module and theme folders were introduced in 1.10 as a configurable item in web.config.

The gulpfile is still only looking in the original hard-coded locations for an assets.json file:

Code:

var assetManifestPaths = glob.sync("Orchard.Web/{Core,Modules,Themes}/*/Assets.json");
rtpHarry commented 8 years ago

@DanielStolt

Skrypt commented 8 years ago

It is up to each Themes providers to implement the new gulp pipeline if they feel that they need it. I would strongly suggest it. It should looks for any Core,Modules,Themes folders that have an Assets.json file.

Ex : /Orchard.Web/Themes/MyTheme/Assets.json /Orchard.Web/Modules/MyModule/Assets.json /Orchard.Web/Core/CoreExtension/Assets.json

/*/ --> this means any "folder name"

rtpHarry commented 8 years ago

But the new feature means they might not be in those three core folders:

https://github.com/OrchardCMS/Orchard/blob/dev/src/Orchard.Web/Web.config#L17

Web.config snippet:

    <!-- Optional additional comma-separated list of folders for Modules or Themes -->
    <!-- Note: Themes that are not under ~/Themes/ should have web.config with appropriate access permissions. -->

    <!--
    <add key="Modules" value="~/Ibn_Modules,~/Customer1/Modules"/>
    <add key="Themes" value="~/Themes/MoreThemes,~/Customer1/Themes" />
Skrypt commented 8 years ago

If you "move, add" the module or theme folders you should then change the gulpfile.js accordingly. Else we would need to generate the gulpfile.js ... not sure we want to do that. Leaving @DanielStolt give his opinion on that one :)

rtpHarry commented 8 years ago

yep... we are going to end up with an asset manifest for the asset manifests :)

CSurieux commented 8 years ago

This system has so many problems. The main ones for me are:

sfmskywalker commented 8 years ago

Is it the system, though?

CSurieux commented 8 years ago

Well if you succeed with "watch" array applied in imported LESS file, I would be pleased to know the trick to use. Concerning your link, this option it is too much restrictive and does not apply For the dummy files, I am using this asset folder in 2% of my modules, so not generating it seems a better option and if I need it I will create just as for the Driver or Handler folders which are not created by default. Moreover, if the folder is created do not fill it with a dummy less code which generates empty css which is loaded in the features in many versions.

DaRosenberg commented 8 years ago

@CSurieux as usual you are trolling and polluting the thread with irrelevant nagging. Open separate issues for your complaints, because they have nothing to do with this one.

DaRosenberg commented 8 years ago

As to the actual topic - I do agree that it would be useful if the gulp pipeline handled this automatically, but I'm struggling a bit to see a robust/clean way to do it.

The reason why we constrain the glob pattern to Orchard.Web/{Core,Modules,Themes}/*/Assets.json is performance, particularly with respect to watchers - so I suppose we could remove that constraint to look for Asset.json files across the whole solution which would solve this for some scenarios. But in Web.config I assume we can also specify other arbitrary folders that could, theoretically, be anywhere on disk? Those cases would not be handled...

One idea could be to have Gulpfile.js actually parse out the configured extension folders from Web.config, and add them to the glob. Might not be too difficult actually. Would be interested to hear what people (non-trolls) think about that?

CSurieux commented 8 years ago

@DanielStolt too easy to always close the discussion to the scope you decide and distribute Troll tags when you don't know. If I am a troll you are the troll master. You could minimally agree on the fact that what I say is true: for people using legacy Orchard features and Less construction there are very important flawns.

sfmskywalker commented 8 years ago

@CSurieux But @DanielStolt is right; we are off-topic. Better to open a discussion thread elsewhere and stay focused on the issues at hand.

CSurieux commented 8 years ago

Sorry but I don't totally agree on the perimeter given to issues, sometime the whole thing must be re-thinked rather than adding micro and complex evolutions which block the real better way. And I am tired of this lack of ... how to call this ???

Skrypt commented 8 years ago

You can add a "watch" array of .LESS files to monitor for changes so that you can structure your LESS code

{
    "inputs": [
        "Assets/Theme/Less/includes/themes/theme-orchard.less"
    ],
    "output": "Styles/TheAdmin.css",
    "watch": ["Assets/Theme/**/*.less"]
},

Here is the example that we use in the new admin theme. @CSurieux you should look at it.

CSurieux commented 8 years ago

Sorry but I have done this 6 months ago and it does not work

Skrypt commented 8 years ago

It does in the new admin theme. Just try it !

CSurieux commented 8 years ago

It is still in my json and does not. You have no import statements. I am using a common.less used in 4 different final targets and some local imports for each target and it does not work, I am always obliged to touch the importing less file to have final css's built.

Skrypt commented 8 years ago

Open an other issue, we will look into it. I'm willing to help you out to make it work. We have @imports in other files that you did'nt looked into.

For the current topic, I think how the gulpfile.js is right now is fine. Having it in sync with the web.config file would'nt hurt but still I think that we will never cover all scenario's possible just by doing that. Example : someone could want to put the module or theme folder out of the /Orchard.Web/ folder ... Wich makes me think that what we need is to add this in the documentation so that everyone knows how to work with it.

CSurieux commented 8 years ago

This system of fragmenting issues to focus on a clear problem is like the whole gulp and .less import flawn. I have already opened an issue on this. After years following Orchard, I am sure the issue management when it tries to behave as in stackoverflow is not good . It is a good process for them, they don't deal with One project but with customers and their target is to have solved on each issue. In a real project, this is not the target. You should not work with a sole process of having a long list of issues and killing one after the other. Some collation and aggregation process is necessary. This is the reason why fragmenting issues is not good. As the Codd rules for relational databases are not the best thing when in real universe. Ok, let's stop this.

bleroy commented 8 years ago

You seem to be arguing against open source, or at least against the collaborative model where everyone comes in with different priorities.To be more constructive, you can very well refer to one issue from another, giving proper perspective. I would reinforce that one issue per problem is the right way to go however.

CSurieux commented 8 years ago

If you want we open something somewhere because I agree we are no more on the gulp system Pb. I am not arguing against open source ? Why are you saying this ?

Skrypt commented 8 years ago

Alright in the new admin theme you have. \Orchard.Web\Themes\TheAdmin\Assets\Theme\Less\includes\themes\theme-orchard.less file wich is the entry point for the LESS transpiler. In this file you have @import "../../orchard.less"; wich is on the first line of that file. Then, if we go see in that file orchard.less file. We have again a lot of @import

@import "style.less";
@import "orchard-media-library.less";
@import "orchard-flexbox-common.less";
@import "orchard-alerts.less";
@import "orchard-navigation.less";
@import "orchard-widgets.less";
@import "orchard-modules.less";
@import "orchard-structure.less";
@import "orchard-panels.less";
@import "orchard-tables.less";
@import "orchard-tabs.less";
@import "orchard-forms.less";
@import "orchard-pager.less";
@import "orchard-modal.less";
@import "orchard-content-items.less";
@import "orchard-admin-shim.less";
@import "orchard-voffset.less";

These are the LESS files that are structured per scope/functionalities.

If you look into the \Orchard.Web\Themes\TheAdmin\Assets.json file you will see :

"watch": ["Assets/Theme/**/*.less"]

So as long as people uses this folder to add/edit LESS files the gulp watch task will trigger a "transpilation" of those files. If you ommit to add the @import of a newly create LESS file then it will not add it to the transpiled \Orchard.Web\Themes\TheAdmin\Styles\TheAdmin.css

I've not had any problems to work like that. I could even add files to watch if needed.

TODO : Add proper documentation.

CSurieux commented 8 years ago

assets.json:

[ { "inputs": [ "Assets/Less/Lib/Bootstrap/bootstrap.less", "Assets/Less/Lib/FontAwesome/font-awesome.less" ], "output": "Styles/Lib.css", "watch": [ "Assets/Less/Lib/Bootstrap/variables.less" ] }, { "inputs": [ "Assets/Less/Site.less" ], "output": "Styles/Site.css", "watch": [ "Assets/Less/common.less","Assets/Less/layout.less", "Assets/Less/bootswatch.less" , "Assets/Less/lib/bootstrap/variables.less" ] },
{ "inputs": [ "Assets/Less/Home.less" ], "output": "Styles/Home.css", "watch": [ "Assets/Less/common.less", "Assets/Less/layout.less", "Assets/Less/bootswatch.less" , "Assets/Less/lib/bootstrap/variables.less" ] },
{ "inputs": [ "Assets/Less/Blogs.less" ], "output": "Styles/Blogs.css", "watch": [ "Assets/Less/common.less","Assets/Less/layout.less", "Assets/Less/bootswatch.less", "Assets/Less/lib/bootstrap/variables.less" ] },
{ "inputs": [ "Assets/Less/Logon.less" ], "output": "Styles/Logon.css", "watch": [ "Assets/Less/common.less","Assets/Less/layout.less", "Assets/Less/bootswatch.less", "Assets/Less/lib/bootstrap/variables.less" ] } ]

This does not watch ?

Skrypt commented 8 years ago

I can assure you that this should work if everything is at the right place. Try finding an error in the LESS files that would prevent the transpilation to kick in. Try to also look if you don't have circular @import in those LESS files that could prevent this to work. I'm not totally sure why you are watching the Bootstrap variables.less file directly. Those Bootstrap files should never be edited, only overriden by another LESS file that is declared later on in the CSS pipeline.

CSurieux commented 8 years ago

No circular reference !!!!, no error, it is a well known pb of gulp. Sorry but I am not interested in "watch": ["Assets/Theme/*/.less"] which rebuild everything as soon as one file is changed.

bleroy commented 8 years ago

When you said "After years following Orchard, I am sure the issue management when it tries to behave as in stackoverflow is not good . It is a good process for them, they don't deal with One project but with customers and their target is to have solved on each issue." it sounded to me like you were arguing against a collaborative model where everyone has their own set of priorities. Then again, it wasn't super clear, so I may have misinterpreted. In any case, we all prefer to work with well-defined, single-responsibility issues. If something needs context from other issues, you can cross-reference them. It's the first time I see anyone argue for multiple issue tickets, anywhere.

CSurieux commented 8 years ago

@Jasmin I am using bootswatch. Anyway thanks for your efforts, I can't keep on. @Bertrand it is not multiple issues tickets, just a perimeter and a correlation factor, relating tickets to each others is not working perfectly. May be you have not seen this, but I have, in large telco projects years ago and ... last year for a bank with 1200 tickets backlog. The consulting company was taking the work ticket by ticket because all the statistics where bulit on the number of solved/backlog tickets. There are many ways to manage this problem. It seems that today developers are very influenced by some large boards as Stackoverflow, forgeting they have differents targets. Not to say that stackoverflow is bad. Simply they have not the same targets as us, they have no way to process differently.

bleroy commented 8 years ago

I want to reassure you that nobody's making job-altering statistics on solved and backlog tickets. The concern is just to avoid those situations where we agree on only part of the issue and/or solution, and have tickets that remain open because they are only partly fixed. Single issues also are easier to understand, making it more likely that somebody will pick it up and work on it. For those broader scenarios that you seem to be talking about, I think it's fine to still break it down into atomic problems, and then have one "meta-issue" that gives the broader context and references each of the smaller issues. Would that be an improvement?

Sorry by the way for derailing the thread a bit.This probably belongs on forums.

Skrypt commented 8 years ago

We could relate what you said with the content cloning PR that is currently investigated wich solves a lot of different issues. #6630

I used "Localization" label to regroup all these issues so that I can see a better global scope of what's missing.

Yes, you are using Bootswatch wich should have their own overrides of the main Bootstrap variables. I would add a watch on those. Example : https://bootswatch.com/cosmo/variables.less

DaRosenberg commented 8 years ago

@CSurieux Nobody is saying that an issue can't be big and composite, or have a broad scope. Indeed there are plenty of issues created with very broad scopes - that's fine!

What we are asking is simply that you refrain from trying to change the scope of an issue somebody else created. Don't hi-jack an issue created with a specific narrow scope to try and promote your broader thoughts, because when you do that the OPs intended issue gets lost in the noise - instead, create a broader issue of your own, and reference this one as an example.

As a case in point - I think very few people actually noticed my response to the issue at hand (which was actually requested) because everyone has been caught up in this meta-discussion with you.

CSurieux commented 8 years ago

@Jasmin Savard in my model, using one bootswatch theme, it is replaces bootstrap, you have not bootstrap AND bootswatch side by side. Are you learning when teaching the others :), more consulting guy ?

CSurieux commented 8 years ago

@Bertrand Ok

Skrypt commented 8 years ago

Here an example of implementation of Bootswatch : https://github.com/psenechal/PJS.Bootstrap/tree/master/Styles We can see that clearly the guy is using Bootstrap lib and also Bootswatch specific less files. Look at how he did this. You are supposed to have Bootstrap and Bootswatch side by side. image

CSurieux commented 8 years ago

Thanks but I implemented correctly and have been using it since a long time. May be before Philippe. Do a try on your side. I have no more time to help you. Best.

rtpHarry commented 8 years ago

@DanielStolt

The reason why we constrain the glob pattern to Orchard.Web/{Core,Modules,Themes}/*/Assets.json is performance, particularly with respect to watchers - so I suppose we could remove that constraint to look for Asset.json files across the whole solution which would solve this for some scenarios. But in Web.config I assume we can also specify other arbitrary folders that could, theoretically, be anywhere on disk? Those cases would not be handled...

I think it would be reasonable to require extra configuration if your module folder is outside of the root. The examples given in the web.config both have ~ notation at the start. I think to even use external folders you would have to configure special permissions to access the file system so you must at that point be expecting to have to do extra configuration elsewhere.

If we can't pull this information automatically then we will need to provide some kind of registration mechanism so that the gulpfile can have extra locations injected without having to modify the gulpfile directly. It might end up with yet another manifest / config file but I guess in the majority of situations it wouldn't be required.

One idea could be to have Gulpfile.js actually parse out the configured extension folders from Web.config, and add them to the glob. Might not be too difficult actually. Would be interested to hear what people (non-trolls) think about that?

I think this is a good option to explore. I don't know what the complexities of resolving the ~ is within JavaScript?

rtpHarry commented 8 years ago

I was just re-reading this thread to write some more of the docs and saw this comment by Bertrand

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.

I forgot about this but putting your own proprietary extensions into this file is allowed and in fact encouraged by the NPM team as they wanted it to become the standard js package format but still be flexible for people's needs.

This could be the location for the extra paths to be stored if we can't calculate them automatically.

DaRosenberg commented 8 years ago

@rtpHarry Interesting - which package.json file would be put the paths in?

Skrypt commented 8 years ago

I would add an empty assetConfig.json file to the solution items/gulp folder or use the package.json file "wich I think is less explanatory". Load the assetManifestPaths from that json wich would contain by default the paths to the "core, module, themes" folder. We should also let people be able to define wathever paths they like to watch. Wich means to not be restrained to "Orchard.Web/" folder.

Could be done totally differently, but the idea is to be able to have a list of unrestrained, editable paths that we can add to the gulp watcher.

rtpHarry commented 8 years ago

@DanielStolt there is just one package.json isn't there? Are you mixing it up with nuget's packages.config, which are all over the place?

@Skrypt Yes, both approaches are equal from a technology standpoint. I guess it just comes down to the opinion of the group as to whether we want more file formats or not. The only reason I was looking for an existing place to stuff the config was because I thought that the consensus was that Orchard already had too many config files and formats. If somebody can make a compelling case for one or the other then I'm happy with that.

DaRosenberg commented 8 years ago

@rtpHarry I don't know what I was thinking... yes, I somehow confused the two.

Anyway, it would be nice if we could avoid forcing folks to alter files that are part of the Orchard code base, and not have to deal with constant merging down the line when pulling in newer versions of Orchard into their repos. So that would point to a solution where we allow people to add files to the solution which we automatically discover.

On the other hand, such a solution is less discoverable because the file is not there to begin with - how do you know what to add and where? Also, it's dangerously close to the "manifest for manifests" vision someone mentioned above.

So perhaps a reasonable compromize is to put this into a file that does exist in the Orchard repo, but which rarely (if ever) changes there, but rather will only ever change in your own repo. To that point, I think an assetsConfig.json would be better than packages.json because a) it is more discoverable and self-explanatory - if you're looking for some place to configure the asset stuff, you would see that file and make the connection and b) it is much less likely to change in the Orchard repo for any reason, in fact it's hard to see why it ever would.

So my vote: we add an assetsConfig.json file next to Gulpfile.js and we move the default paths into that file.

rtpHarry commented 8 years ago

ok yes assetsConfig.json sounds like the winner then.

Although we haven't had much of a go at auto-discovery yet. If the gulpfile.js can find the web.config and open it then it knows that ~ means that current folder.

  1. open web.config, extract key(s), split into array
  2. If it starts with ~: replace the tilde with the relative path from gulpfile.js to web.config. add glob pattern on end.
  3. If its a relative path then thats fine, same as above It might go up and down the file tree but thats not an issue.
  4. If its a file path then I assume node will be ok with it? needs testing

At the moment there could potentially be two locations for web.config, if they are using the azure project but Seb said that was being phased out shortly.

sebastienros commented 8 years ago

There is just one web.config on the dev branch right now.

DaRosenberg commented 8 years ago

That would work I guess - and as @sebastienros pointed out, the Azure solution is already long gone in dev.

But I somehow don't like the idea anymore, it seems messy somehow. The assetsConfig.json approach seems nicer to me, I like that asset manifest harvesting is a separate, explicitly configured process that does not have to necessarily align 1-to-1 with extension folders. For example, maybe you have assets located somewhere completely different, such as in your Media folder, that you wish to process? This way you can do that, and process any assets even if they don't reside in an Orchard extension proper.

rtpHarry commented 8 years ago

maybe a bool setting in assetconfig that says "auto harvest modules from custom folders defined in web.config" which defaults to true. That way for 90% of the users it just works, and it can be tweaked for advanced users without having to touch the gulpfile.

DaRosenberg commented 8 years ago

That's not bad. I like it. You want to take a stab at it?

rtpHarry commented 8 years ago

Yep I'll take a run at it

rtpHarry commented 8 years ago

Code is messy at the moment but its being worked on.

The js is here but not been merged into the main gulpfile.js yet as there is still refactoring to do.

It introduces a new assetsConfig.json as agreed which pulls in from the web.config and also allows custom paths to be added into the pipeline.

DaRosenberg commented 8 years ago

One bif of feedback on what you have so far: I think we should keep the actual logic to get paths from Web.config in the main Gulpfile.js and let assetsConfig.json only contain the actual paths, along with the boolean to control whether to read Web.config. This is to minimize the risk of people having to deal with conflicts when they've made changes, by keeping the stuff we might change in one file and the stuff they might change in another file.

rtpHarry commented 8 years ago

Yeah you mean it shouldn't stay in parseWebConfig.js?

That was just so I could debug it in Node while I was writing it without the rest of the pipeline getting in the way. I agree it should be merged.

DaRosenberg commented 8 years ago

doh this is the second time I've confused concepts in this thread. I somehow had it in my head that you might intend to keep the logic in the JSON file, not even realizing the impossibility of this... Please disregard my silliness.