Skip to content
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

Rebuild all assets #15772

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions .github/workflows/assets_validation.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
name: Frontend Assets Build Validation
name: Assets Build Validation
on:
# Manual trigger.
workflow_dispatch:
push:
paths-ignore:
- '**/*.md'
- 'mkdocs.yml'
- 'src/docs/**/*'
branches: [ main ]
pull_request:
branches: [ main, release/** ]
concurrency:
group: ${{ github.head_ref || github.run_id }}
cancel-in-progress: true
jobs:
test-npm-build:
name: Test building assets
Expand All @@ -21,16 +32,17 @@ jobs:
- name: Check if git has changes
shell: pwsh
run: |
git status
$changes = git status --porcelain

if ($changes)
if ([string]::IsNullOrEmpty($changes))
{
Write-Host "No uncommitted changes found. Repository is clean."
}
else
{
Write-Output 'Please make sure to build the assets properly before pushing, see https://docs.orchardcore.net/en/latest/docs/guides/gulp-pipeline/.'
Write-Output 'The following files changed:'
Write-Output $changes
exit -1
}
else
{
Write-Host "No uncommitted changes found. Repository is clean."
}
14 changes: 11 additions & 3 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ var fs = require("graceful-fs"),
util = require('gulp-util'),
postcss = require('gulp-postcss'),
rtl = require('postcss-rtl'),
babel = require('gulp-babel');
babel = require('gulp-babel'),
sort = require('gulp-sort');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gulp-sort is unmaintained, with the last release 8 years ago. This is why I went with manual storing under #15735 (and we should have a better long-term solution anyway: #15740).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorting them manually will work too. The trick is to sort the files after they are resolved.


// For compat with older versions of Node.js.
require("es6-promise").polyfill();
Expand Down Expand Up @@ -108,15 +109,17 @@ function getAssetGroups() {
assetGroups.push(assetGroup);
});
});

return assetGroups;
}

function resolveAssetGroupPaths(assetGroup, assetManifestPath) {
assetGroup.manifestPath = assetManifestPath;
assetGroup.basePath = path.dirname(assetManifestPath);
assetGroup.inputPaths = assetGroup.inputs.map(function (inputPath) {
const inputPaths = assetGroup.inputs.map(function (inputPath) {
return path.resolve(path.join(assetGroup.basePath, inputPath)).replace(/\\/g, '/');
});
assetGroup.inputPaths = inputPaths.sort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already about hard-coded input paths (right?) we needn't sort them. Or rather, we mustn't sort them, since the manually specified order might matter in the resulting bundle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't think this sort is needed because we sort later after grabbing the files gulp.src. If you want, remove that sort but keep the other and see if we still get the same results. If that does not work, you can re-add it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works without sorting at all in the gulpfile too (see #15772 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it will work without some sort of sort. Because when we use path that contains a star, some tasks may resolve slower than other which cause the concatenation to not be consistent. When you force the sort, this ensures that the files are always concatenated in the same order. This leads to generating the same file over time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I mean, this is why we need manual sorting, but in the Assets files. The gulpfile needn't know about that.

This isn't an ideal solution (the whole assets pipeline is far from ideal), but without fundamentally re

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three cases of a wildcard lookup in OC that needed the paths manually listed. Until we add any new files in those three folders, we're done. And if we do add new files there, it's about adding that path to the Assets file (i.e. the same as in the other cases where there's no wildcard). I think we can live with that, and I'd prefer this as an interim solution rather than relying on a library that was last updated two major Gulp versions prior.

And the Gulp pipeline is already riddled with issues:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on a better solution tomorrow. We should support ** instead of dropping the support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 places are where it was used and the order matters, ever (since this only affects the OC source). Why do we need anything more complex before we fix this for good and delete this anyway?

If we were to keep this process for the foreseeable future then I wouldn't be happy with this either. But I don't see the point in trying to patch a fundamentally outdated system to maybe prevent a 5-second inconvenience in the future if somebody adds new files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Piedone look at the latest comment in this PR. I fixed the issue without having to depend on any new library.

change the resolveAssetGroupPaths function to this

function resolveAssetGroupPaths(assetGroup, assetManifestPath) {
    assetGroup.manifestPath = assetManifestPath.replace(/\\/g, '/');
    assetGroup.basePath = path.dirname(assetGroup.manifestPath);
    var inputPaths = assetGroup.inputs.map(function (inputPath) {
        return path.resolve(path.join(assetGroup.basePath, inputPath)).replace(/\\/g, '/');
    });

    // get all input paths and then sort them to ensure file concatenation is consistent.
    assetGroup.inputPaths = glob.sync(inputPaths, {}).sort();

    assetGroup.watchPaths = [];
    if (!!assetGroup.watch) {
        assetGroup.watchPaths = assetGroup.watch.map(function (watchPath) {
            return path.resolve(path.join(assetGroup.basePath, watchPath)).replace(/\\/g, '/');
        });
    }
    assetGroup.outputPath = path.resolve(path.join(assetGroup.basePath, assetGroup.output)).replace(/\\/g, '/');
    assetGroup.outputDir = path.dirname(assetGroup.outputPath);
    assetGroup.outputFileName = path.basename(assetGroup.output);
    // Uncomment to copy assets to wwwroot
    //assetGroup.webroot = path.join("./src/OrchardCore.Cms.Web/wwwroot/", path.basename(assetGroup.basePath), path.dirname(assetGroup.output));
}

update glob to "glob": "^10.3.12",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move this to your PR.

assetGroup.watchPaths = [];
if (!!assetGroup.watch) {
assetGroup.watchPaths = assetGroup.watch.map(function (watchPath) {
Expand Down Expand Up @@ -174,6 +177,7 @@ function buildCssPipeline(assetGroup, doConcat, doRebuild) {
generateSourceMaps = false;

var minifiedStream = gulp.src(assetGroup.inputPaths) // Minified output, source mapping completely disabled.
.pipe(sort())
.pipe(gulpif(!doRebuild,
gulpif(doConcat,
newer(assetGroup.outputPath),
Expand Down Expand Up @@ -208,6 +212,7 @@ function buildCssPipeline(assetGroup, doConcat, doRebuild) {
// Uncomment to copy assets to wwwroot
//.pipe(gulp.dest(assetGroup.webroot));
var devStream = gulp.src(assetGroup.inputPaths) // Non-minified output, with source mapping
.pipe(sort())
.pipe(gulpif(!doRebuild,
gulpif(doConcat,
newer(assetGroup.outputPath),
Expand Down Expand Up @@ -255,8 +260,9 @@ function buildJsPipeline(assetGroup, doConcat, doRebuild) {
target: "es5",
};

console.log(assetGroup.inputPaths);
// console.log(assetGroup.inputPaths);
return gulp.src(assetGroup.inputPaths)
.pipe(sort())
.pipe(gulpif(!doRebuild,
gulpif(doConcat,
newer(assetGroup.outputPath),
Expand Down Expand Up @@ -301,6 +307,8 @@ function buildJsPipeline(assetGroup, doConcat, doRebuild) {
function buildCopyPipeline(assetGroup, doRebuild) {
var stream = gulp.src(assetGroup.inputPaths);

stream = stream.pipe(sort());

if (!doRebuild) {
stream = stream.pipe(newer(assetGroup.outputDir))
}
Expand Down
10 changes: 10 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"postcss": "8.4.28",
"postcss-rtl": "^2.0.0",
"rtlcss": "4.1.0",
"source-map": "^0.7.4",
"typescript": "^5.2.2",
"source-map": "^0.7.4"
"gulp-sort": "^2.0.0"
}
}

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Loading
Loading