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] Generate source maps for bundles #695

Merged
merged 33 commits into from Feb 23, 2022
Merged

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Jan 26, 2022

Supersedes #282

Based on SAP/ui5-tooling#583

JIRA: CPOUI5FOUNDATION-434

@coveralls
Copy link

coveralls commented Feb 8, 2022

Coverage Status

Coverage decreased (-0.1%) to 94.672% when pulling a6e2f06 on feature-bundle-source-maps into 3b51c1b on next.

lib/lbt/bundle/Builder.js Outdated Show resolved Hide resolved
lib/lbt/bundle/Builder.js Outdated Show resolved Hide resolved
@RandomByte RandomByte marked this pull request as ready for review February 22, 2022 15:01
@RandomByte RandomByte merged commit 8a20c42 into next Feb 23, 2022
@RandomByte RandomByte deleted the feature-bundle-source-maps branch February 23, 2022 17:35
@@ -90,6 +90,8 @@ const log = require("@ui5/logger").getLogger("builder:processors:bundlers:module
* @public
* @typedef {object} ModuleBundleOptions
* @property {boolean} [optimize=true] Whether the module bundle gets minified
* @property {boolean} [sourceMap] Whether to generate a source map file for the bundle.
* Defaults to true if <code>optimize</code> is set to true
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct anymore. A sourcemap is always created unless sourceMap: false is set.

In addition: Is this something to be added to the project configuration schema?

Copy link
Member

Choose a reason for hiding this comment

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

The default will be updated via #709

Copy link
Member

Choose a reason for hiding this comment

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

I've added a bullet-point for adding 'sourceMap' to the specVersion 3.0 bundleOptions to SAP/ui5-tooling#506

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, adding this to the bundleOptions makes sense

* @param {ModuleBundleDefinition} parameters.options.bundleDefinition Module bundle definition
* @param {ModuleBundleOptions} [parameters.options.bundleOptions] Module bundle options
* @returns {Promise<module:@ui5/fs.Resource[]>} Promise resolving with module bundle resources
* @returns {Promise<module:@ui5/builder.processors.MinifierResult[]>} Promise resolving with module bundle resources
Copy link
Member

Choose a reason for hiding this comment

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

ModuleBundlerResult

Copy link
Member

Choose a reason for hiding this comment

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

Will be solved via #709

@@ -34,10 +34,12 @@ const debugFileRegex = /((?:\.view|\.fragment|\.controller|\.designtime|\.suppor
* @alias module:@ui5/builder.processors.minifier
* @param {object} parameters Parameters
* @param {module:@ui5/fs.Resource[]} parameters.resources List of resources to be processed
* @param {boolean} [parameters.addSourceMappingUrl=true]
Copy link
Member

Choose a reason for hiding this comment

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

Usually in our processors we have an options object that contains such parameters.

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, I forgot about that. This should be changed then 👍

Copy link
Member

Choose a reason for hiding this comment

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

Fixed via #709

.then((processedResources) => {
return Promise.all(processedResources.map((resource) => {
.then((results) => {
const bundles = Array.prototype.concat.apply([], results);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this is needed in here

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 wonder the same 🤷

Comment on lines +115 to +130
// For "unoptimized" bundles, the non-debug files have already been filtered out above.
// Now we need to create a mapping from the debug-variant resource path to the respective module name,
// which is basically the non-debug resource path, minus the "/resources/"" prefix.
// This mapping overwrites internal logic of the LocatorResourcePool which would otherwise determine
// the module name from the resource path, which would contain "-dbg" in this case. That would be
// incorrect since debug-variants should still keep the original module name.
for (let i = unoptimizedResources.length - 1; i >= 0; i--) {
const resourcePath = unoptimizedResources[i].getPath();
if (taskUtil.getTag(resourcePath, taskUtil.STANDARD_TAGS.IsDebugVariant)) {
const nonDbgPath = ModuleName.getNonDebugName(resourcePath);
if (!nonDbgPath) {
throw new Error(`Failed to resolve non-debug name for ${resourcePath}`);
}
unoptimizedModuleNameMapping[resourcePath] = nonDbgPath.slice("/resources/".length);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We have this code in three places now, so it would be better to refactor it

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, maybe we could have all bundling tasks call the generateBundle-task or create a "helper" module in lib/tasks/bundlers/ for this purpose 👍

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.

None yet

4 participants