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

[FEATURE] Add minify task and processor #666

Merged
merged 25 commits into from
Jan 17, 2022
Merged

[FEATURE] Add minify task and processor #666

merged 25 commits into from
Jan 17, 2022

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Nov 23, 2021

Implements SAP/ui5-tooling#583 based on #657

Depends on SAP/ui5-fs#331

  • Combine debug-file creation, minification and source-map generation into a single minify task
    • Replaces existing uglify and createDebugFiles tasks
  • Tag debug-files with new "global" (=shared across projects) tag IsDebugVariant
  • Filter debug-files from moduleBundler so that it only packages minified or debug-sources (depending on optimize bundleOption)

Open TODOs:

  • Add integration test (or modify an existing one) for optimize: true bundle generation without minify task
  • Add integration test (or modify an existing one) for optimize: false bundle generation with minify task

@RandomByte RandomByte force-pushed the minify-task branch 2 times, most recently from 0877d46 to 167af6b Compare November 25, 2021 12:27
test/lib/index.js Outdated Show resolved Hide resolved
@RandomByte RandomByte marked this pull request as ready for review December 3, 2021 08:51
@RandomByte RandomByte force-pushed the minify-task branch 2 times, most recently from fb813f6 to 9b5eddf Compare December 22, 2021 16:18
@RandomByte
Copy link
Member Author

Tests are failing because of dependency to SAP/ui5-fs#331

@@ -2,6 +2,9 @@ const ResourcePool = require("./ResourcePool");
const LocatorResource = require("./LocatorResource");

class LocatorResourcePool extends ResourcePool {
constructor({ignoreMissingModules} = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -58,7 +59,7 @@ function scanFileOrDir(fileOrDir, name, pool) {
*/

async function determineDependencyInfo(resource, rawInfo, pool) {
const info = new ModuleInfo(resource.name);
const info = new ModuleInfo(ModuleName.getNonDebugName(resource.name) || resource.name);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

dbgResource.setPath(dbgPath);

const filename = path.posix.basename(resource.getPath());
if (filename.endsWith(".support.js")) {
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this is the reason why it should not be done

Suggested change
if (filename.endsWith(".support.js")) {
// Support rules should not be minified to have readable code in the Support Assistant
if (filename.endsWith(".support.js")) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, moved to glob pattern in Application- and LibraryBuilder


return Promise.all(processedResources.map(async ({resource, dbgResource, sourceMapResource}) => {
if (taskUtil) {
taskUtil.setTag(resource, taskUtil.STANDARD_TAGS.HasDebugVariant);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really correct for the "*.support.js" files?
I would expect that those should be skipped completely (no tags set).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

t.deepEqual(await resources[3].resource.getPath(), "/test4.support.js",
"Correct resource path for minified content of resource 4");
t.deepEqual(await resources[3].resource.getString(), expectedMinified4, "Correct minified content for resource 4");
t.deepEqual(await resources[3].dbgResource.getPath(), "/test4-dbg.support.js",
Copy link
Member

Choose a reason for hiding this comment

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

This assertion should fail, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not relevant anymore, minifier now has no special handling of support files.

`{"version":3,"sources":["test3-dbg.designtime.js"],"names":["test3","paramA","variableA","console","log"],` +
`"mappings":"AACA,SAASA,MAAMC,GACd,IAAIC,EAAYD,EAChBE,QAAQC,IAAIF,GAEbF","file":"test3.designtime.js"}`;

t.deepEqual(await resources[0].resource.getPath(), "/test1.controller.js",
Copy link
Member

Choose a reason for hiding this comment

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

getPath isn't async, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 316 to 318
}).transformer(async function(resourcePath, getClonedResource) {
if (taskUtil.getTag(resourcePath, taskUtil.STANDARD_TAGS.IsDebugVariant)) {
const resource = await getClonedResource();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}).transformer(async function(resourcePath, getClonedResource) {
if (taskUtil.getTag(resourcePath, taskUtil.STANDARD_TAGS.IsDebugVariant)) {
const resource = await getClonedResource();
}).transformer(async function(resourcePath, getResource) {
if (taskUtil.getTag(resourcePath, taskUtil.STANDARD_TAGS.IsDebugVariant)) {
const resource = await getResource();

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -3,9 +3,11 @@ jQuery.sap.registerPreloadedModules({
"version":"2.0",
"modules":{
"application/i/Component.js":function(){sap.ui.define(["sap/ui/core/UIComponent"],function(n){"use strict";return n.extend("application.i.Component",{metadata:{manifest:"json"}})});
//# sourceMappingURL=Component.js.map
Copy link
Member

Choose a reason for hiding this comment

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

Should those comments be part of the preload? If the browser ignores them I guess it's okay. If it causes problems, we should remove them.

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 think they are not a problem unless they are at the end of the file. Anyways, the source map change for the module bundler will remove them 👍

@@ -0,0 +1,36 @@
/*!
Copy link
Member

Choose a reason for hiding this comment

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

As already mentioned above: We don't want to create -dbg files for *.support.js files. So they should just remain as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@RandomByte RandomByte force-pushed the minify-task branch 2 times, most recently from 8a71cde to a6ad03a Compare December 23, 2021 14:52
Combines debug-file creation, source-map generation and minification.
Replaces existing tasks/processors 'uglify' and 'createDebugFiles'.

BREAKING CHANGE:
The following tasks have been removed:
- createDebugFiles
- uglify

The following processors have been removed:
- debugFileCreator
- resourceCopier
- uglifier

As a replacement, the new 'minify' task and 'minifier' processors can be
used.

Note: The minify task is now executed earlier, before the bundling
process takes place. The 'beforeTask' and 'afterTask' configuration of
custom tasks might need to be adapted to cater for this change.
Bundling can now rely on preceding minify task
…ix error message

Do not check whether a debug-variant already exists. Just overwrite it
Only for bundles configured as 'optimize: false'.

Removes LocatorResourcePool  option to remove -dbg from resource names.
…unoptimized sap.ui.core bundles

Only applicable to sap.ui.core versions <1.97.0. Same logic as for
custom bundles in the generateBundle task.
@coveralls
Copy link

coveralls commented Jan 14, 2022

Coverage Status

Coverage increased (+0.01%) to 94.782% when pulling c0676d6 on minify-task into c110dbc on next.

@RandomByte
Copy link
Member Author

Rebased on top of latest next to get new ui5-fs features

lib/tasks/TaskUtil.js Show resolved Hide resolved
lib/tasks/TaskUtil.js Show resolved Hide resolved
lib/tasks/TaskUtil.js Show resolved Hide resolved
return this._projectBuildContext.getResourceTagCollection().clearTag(resourcePath, tag);
}

getResourceTagCollection() {
Copy link
Member

Choose a reason for hiding this comment

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

But.. is this method actually used somewhere?
I couldn't find a reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this shouldn't be necessary anymore. Probably it was added for some different, intermediate implementation.

@@ -76,12 +77,18 @@ module.exports = async function({workspace, dependencies, taskUtil, options: {pr
`unable to generate complete bundles for such projects.`);
}

// If an application does not have a namespace, its resources are located at the root. Otherwise in /resources
Copy link
Member

Choose a reason for hiding this comment

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

By removing this behavior, we need to make sure to fail in case we don't have a namespace (as already planned for 3.0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct!

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.

3 participants