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

Rebuild all assets #15772

wants to merge 22 commits into from

Conversation

MikeAlhayek
Copy link
Member

Fix #15726

@MikeAlhayek
Copy link
Member Author

@Piedone I think the issue here is that every time we run gulp rebuild new media.*.js, trumbowyg-plugins.*.js and trumbowyg-plugins.*.css are generated. It is strange that each time this run, a minor change is made to the output files.

@MikeAlhayek
Copy link
Member Author

Something about the trumbowyg-plugins.

image

@MikeAlhayek MikeAlhayek requested a review from Piedone April 16, 2024 16:12
@Piedone
Copy link
Member

Piedone commented Apr 16, 2024

I did this already: #15735

@MikeAlhayek
Copy link
Member Author

@Piedone sorry I did not see your PR. The issue with the assets always rebuilding is fixed. but the Assets Build Validation is still failing. Feel free to take the resources fix from here into your PR and close this PR. We still have to fix the assets workflow.

@MikeAlhayek
Copy link
Member Author

@Piedone any idea why the function test in the assets_validation is failing here? I can't make sense of it. Running the sites locally "agency theme" and the blog theme, both appear with no issues locally.

@Piedone
Copy link
Member

Piedone commented Apr 16, 2024

I'll integrate your changes under my PR then, I'll look into it.

@@ -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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend assets are outdated
2 participants