-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Assets Build Tool (Assets Manager) #17262
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # src/OrchardCore.Modules/OrchardCore.AdminDashboard/Assets.json # src/OrchardCore.Modules/OrchardCore.AdminMenu/Assets.json # src/OrchardCore.Modules/OrchardCore.AdminMenu/wwwroot/Scripts/admin-menu-icon-picker.js # src/OrchardCore.Modules/OrchardCore.Media/ResourceManifestOptionsConfiguration.cs # src/OrchardCore.Modules/OrchardCore.Resources/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/OrchardCore.Modules/OrchardCore.AdminDashboard/Assets2.json
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.AdminDashboard/Assets2.json
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.AdminMenu/Views/Menu/List.cshtml
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.AdminMenu/package-lock.json
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Flows/wwwroot/Scripts/flows.edit.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
…MS/OrchardCore into skrypt/asset-build-tool
Alright, I think I pushed enough efforts on that one. Time to approve it and get it merged. |
Will check this out soon. |
@sebastienros I can see only around 30 messages on this one (after expanding everything). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not competent to comment on the asset manager's code, so I focused on trying it out and using it as any contributor, checking if I find any issues, and reviewing the rest.
What do you mean by?
I want to move to File App as soon as possible. That was the original goal.
Please:
- Apply suggested changes directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, and that's fine.
- Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them. There will be a lot here, so this is important, otherwise we won't be able to follow the conversation.
- Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can't migrate them all yet unless refactor them entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why a refactoring is necessary? This is what I see in the Assets.json
files (again not counting Media):
- AdminMenu: the remain Gulp pipeline does bundling of two JS files. As we discussed in a meeting, this is not even necessary with HTTP 2 today, so it could be swapped out with a
ResourceManagementOptionsConfiguration
for the two scripts and a dependency declaration there, but if not, the new Assets Manager supports bundling. Why not just use that? Seems like a 1-1 replacement to me. - Liquid: I suppose this kind of TS compilation is different than the one you migrated in some other projects?
- Resources: SCSS compilation and JS bundling, which are supported by the new Assets Manager 1-1 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to try doing what you just suggested there. On my side, I tried refactoring them using Typescript and compiling them as ES6 assets. Did not work for me so far. We could maybe try compiling these as CommonJS with Parcel, I did not try.
AdminMenu: The remaining asset is the fontawesome iconpicker component and I tried converting it to typescript but it is just not written for doing imports. It uses a JQuery extension method which the TS compiler doesn't understand. Needs to be refactored without JQuery. Else, we could try compiling it as CommonJS with Parcel but I'd rather keep the Gulp pipeline and not waste more time on something that needs a refactor.
Resources: Monaco and Trumbowyg editors mainly which I did not have time to work on trying to make them work. I started and I saw that there was a ton off merged files together. Why wasting time on migrating that stuff to the new Asset Manager if we don't refactor it using typescript? That's waste of time and there's nothing wrong with the gulp pipeline... it still works ... it is just old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also, I think we will still need to keep the Gulp pipeline up untill we figure out how to compile the Monaco, Liquid and Trumbowyg editors. They all have customizations for OC that I don't know how to recode because I don't know the required specs for them yet. This will take time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Else, when we have single .js files I'm simply using "min" action which doesn't involve any compiler.
But when we "merge" files together then the compiler doesn't know which file should be used first unless I would send an array of files. The current Asset Manager doesn't support array of files because there is no need when using proper "imports" with ES6 scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so distinction to make. Parcel, Vite, Webpack, Rollup are all compilers. Which means they will compile everything that is Ecmascript. They are also bundlers but they are mainly compilers/transpilers. So, this is why I'm refactoring to Typescript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Parcel maybe needs some tweaks to handle better CommonJS use cases.
Other options: Webpack seems to allow to only bundle files... not sure yet.
Either I'm pushing time on refactoring these assets or adding more tweaks to the Asset Manager.
I don't think we will need to support merging files together in the future if we refactor these scripts ...
The issue is that merging files together has never really been a compiler thing.
CommonJS modules normally uses "require"
const circle = require('./circle.js');`
Then the compiler will merge this file with the one where it is written. This is how "bundling" works.
Ecmascript will use "import"
import { modal } from 'boostrap'
import * from 'vue'
But here there is no path involved in this. The compiler will resolve the dependency from the package.json file(s) of your app. And probably also do some tree shaking on the dependency graph.
So, @Piedone here we are making a mistake of thinking that merging 2 files together like gulp does is bundling. It is not bundling at all. What we did with gulp is simply opening a writer and creating a new files out of multiple files without involving any compiler. Parcel, Vite and Webpack may fail to compile these files properly because of missing imports or require.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the "bundleEntrypoint" with Parcel will not work because it will produce 2 CommonJS modules out of these 2 files.
Modules are scoped meaning that you can't access their method without defining their exports.
https://dev.to/lico/how-to-import-and-export-in-commonjs-and-es-modules-43m1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is why if we use these compilers that we need to refactor the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this Frontend folder for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tempted to name it "Bloom" javascript framework for Orchard Core. That's a folder for shared .ts components/modules. This is where we we will build it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Piedone Let me know for this one. I think that's the most important one in all of your comments.
If "Frontend" is not appropriate I would rename it to some more evident like "OrchardCore.Scripts" or "OrchardCore.Bloom" ... I don't know. "Frontend" seems a bit bad. Never found anything more meaningfull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't commenting on the name, but inquiring what the purpose of the folder and its contents are, I see, so this is intended as a component library. Then I'd rather have them in an .esproj and added to the solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.esproj ? I was asking about the name and if we would prefer something else because it is time now to decide before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not an issue, for me at least. But that this folder is disjointed from the rest of the codebase.
src/OrchardCore.Modules/OrchardCore.AdminMenu/wwwroot/Scripts/admin-menu.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a renamed assets so that it compiles as admin-menu.js file in the wwwroot folder. The actions no longer allows to rename files from source to destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was it renamed from? I don't see it. It's strange because it seems to be only a copy of a jQuery plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right it was a jQuery plugin file which I needed to rename so that it works. I believe the licence is kept in that file so there should be no issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see: originally, there was an Assets/js/jquery.mjs.nestedSortable.js
, and one also in OrchardCore.Menu
. The diff only shows the former being renamed to the latter, hence why this shows as a new file, while it actually isn't.
But now I see that this has 4 copies in the codebase, one in OrchardCore.Resources
, so please use that instead.
src/OrchardCore.Modules/OrchardCore.AdminMenu/wwwroot/Scripts/fontawesome-iconpicker.js
Outdated
Show resolved
Hide resolved
...raphQL/wwwroot/Scripts/node_modules_graphiql_react_dist_brace-fold_es_js.graphiql-orchard.js
Outdated
Show resolved
Hide resolved
@Piedone based on all your comments, I think we will need to have a call instead of me answering on each questions here. Also, I explained on last standups meeting some of your questions already. Maybe you were not there. |
I wasn't there last week. Sure, we can have a call, but please focus on the harder ones, we shouldn't just go through each comment in a call. I've written you on Discord. |
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
…MS/OrchardCore into skrypt/asset-build-tool
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
I meant Media Gallery refactor to Vue 3 and typescript. The goal here with this PR was to create a Build tool that would work for building the new Media Gallery in Orchard Core. And that I would be able to use with CI. I did not intent to start refactoring the entire javascript base code of Orchard Core for that matter. So, this is why I did it partially and pushing work on those remaining module for later on. |
I see, I didn't know you were referring to the Media module, OK. |
Adding fontawesome-iconpicker to OC.Resources
…MS/OrchardCore into skrypt/asset-build-tool
Some extra work for me on Christmas holidays.🎅🏼
Assets Manager
Created by @jptissot
Based on Concurrently and Parcel but can also run Vite and Webpack too. This is a non-opiniated build tool which allows to be extended for any asset compiler/bundler that someone may require.
This build tool essentially uses
Concurrently
instead ofGulp
.Concurrently
, is a concurrent shell runner which allows to trigger any possible shell command. It allows to triggerParcel
,Vite
,Gulp
or any other command that we need for building assets. Everything is written as ES6 modules (.mjs files).Kind of needed for Vue 3 migration because it needs to use either
Vite
orParcel
to build as ES6 modules.Old assets are not compiled as ES6 modules so they don't need these bundlers. For that matter I kept the old gulpfile.js which will be now triggered by
Concurrently
.What needs to be done over time is to migrate these javascript files to ES6 modules (.mjs files or .ts files) so that we can compile them as modules. But that's a migration that will happen over time.
Features
yarn build
yarn build -n module-name
yarn watch -n module-name
.yarn build -g
yarn build -gr
yarn host -n module-name
.yarn clean
. Will also clean parcel-cache folder.yarn build -t tagname
Update
Finally, I'm keeping the old gulp pipeline because there is nothing wrong with it. Also, for backward compatibility with older modules that requires it. I'm using
GulpAssets.json
for the Gulp build tool and usingAssets.json
for the new one. This way, no need to migrate to the new build tool. I'm simply triggeringgulp rebuild
withConcurrently
in the new asset build tool. Because that's what it is essentially, a concurrent shell runner. This way, it is going to be a softer migration for those who already have modules that are built with the old gulp pipeline.Of course, this PR needs to be accepted by community first.
TODO
Modules to migrate:
@jptissot @deanmarcussen @aderderian
Fixes #14169. Related to #15740.