[FEATURE] Sourcemap support in the ui5-builder preload generation #282

nlunets · 2019-07-04T14:03:59Z

Always go for the predefine rewrite in the library build
Change the order of the steps to have debug file created early enough
Sourcemap support for the rewrite and for the preload generation
Uglifier can take input sourcemap as well (if you have a custom transpile step...)

I still need to get around to fixing all the tests and usage but if you already wanted to have a look @codeworrior ;)

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

Reviewed the Contributing Guidelines
- Especially the How to Contribute section
No merge commits
Correct commit message style

CLAassistant · 2019-07-04T14:04:23Z

All committers have signed the CLA.

codeworrior

How do you avoid writing out the *.map for the minified individual files? I missed that part...

codeworrior · 2019-07-04T17:38:24Z

lib/lbt/bundle/BundleWriter.js

@@ -25,6 +29,8 @@ class BundleWriter {
 	write(...str) {
 		for ( let i = 0; i < str.length; i++ ) {
 			this.buf += str[i];
+			this.lineOffset += str[i].split(NL).length - 1;
+			this.columnOffset += str[i].length;


shouldn't this just be the length of the last line if there's any NL in the str and be incremented, when there's no NL?

Yes, you're correct, it works today because I just deal with minified, one line files ;)

codeworrior · 2019-07-04T17:44:37Z

lib/lbt/bundle/Builder.js

 			// rewrite sap.ui.define to sap.ui.predefine
 			if ( defineCall.callee.type === Syntax.MemberExpression
 					&& defineCall.callee.property.type === Syntax.Identifier
-					&& defineCall.callee.property.name === "define" ) {
+				&& defineCall.callee.property.name === "define") {


move back to original position , please

if you had prettier we wouldn't have this conversation ! (just teasing, yes I'll move it back)

So 'prettier' basically is a tool that keeps people from talking to each other, did I get that right?

nlunets · 2019-07-04T21:42:49Z

How do you avoid writing out the *.map for the minified individual files? I missed that part...

I do not avoid creating the map for the individual minified files, I actually want that, but that whole part is weird anyway.
Why do we create uglified files if anyway we're not reusing them for the preload and doing it again ?
Also shouldn't we minify the whole preload rather than each file individually in the preload ?

codeworrior · 2019-07-05T06:00:55Z

Having source maps also for each individual source will significantly increase our footprint for the DIST layer, I would assume (once we use the tooling also for DIST's production).

Why are we creating minified files if we are not reusing them for the bundles? And why don't we minify the bundle only after creating it? Well, there are many different reasons for that, but at the end this becomes a historically grown list of excuses:

"rewriting sap.ui.define to sap.ui.predefine" only for the preloads and "sorting by copyright" was easier that way in Maven
we anyhow had no multi-stage source map creation there which would be a precondition
minifying individual files payed out on mobile devices when running without preloads (h2). The memory kept by source code is a significant part of our heap (at least with dev tools open, we never measured without)
we got feedback that some developers prefer the ui5 debug mode over source maps (what's the argument here?)
reg. ui5-tooling, it seemed to be a nice quality of UI5 tasks when they're independent from each other (easier reconfiguration). What we envisioned instead of writing out all intermediate results was a kind of resource-related caching in the tooling. The first task that parses a file, can put the resulting AST in a cache and any later task requesting the same AST could take it from there (if the resource did not change in the meantime). Same for uglified code or for the results of the dependency analysis. Well, an idea, not reality...
for the import into the ABAP BSP, minimizing the line-based diff of preloads (between versions) was reported to be beneficial. This was again easier to ensure with a single stage minification, but not impossible without. Well, we anyhow lost that "smart line break" feature with the migration to uglify
we don't know a minifier that could handle mixed content as present in our preloads. So we anyhow would have to minify JSON / XML content first before embedding it into the preload (it's harder to judge the nature of a string literal in the final preload)

...

nlunets · 2019-07-05T07:11:43Z

Hmmm, apart from the increased footprint I don't see any good reason to not include them (esp if you consider h2) but that's your decision in the end.

Regarding minifying twice it's not a big deal really but we could reuse the one that happened before (if it happened) as an optimisation.

You mentioned the abap stuff, won't that increase diff size create issue during transport ?

codeworrior · 2019-07-05T07:19:06Z

Presumably, yes. But for apps, I'm not too much into the details. For the DIST layer, it doesn't play a role as DIST layer was imported into MIME repo (binaries, no diff) and with the universal bootstrap (redirect to cloud), on premise installations don't play the same role as before.

But transports created for apps IMO could suffer from that changed behavior. As long as Fiori apps are released with Maven, we won't notice. I'll ask Fiori and delivery colleagues next week.

The source maps topic, I'll discuss with Peter and Jens Ittel.

nlunets · 2019-07-09T12:54:55Z

I've removed the automatic creation of map for the uglified files now and the test should all run.
I've checked some of the new output but you're the experts ;)

coveralls · 2019-07-09T12:56:24Z

Coverage increased (+0.2%) to 90.527% when pulling 54edf28 on nlunets:sourcemap into 9aa6785 on SAP:master.

nlunets · 2019-09-10T12:15:44Z

@codeworrior This has been rebased and should be fine for review "code wise".
I'll check separately that the sourcemap are actually correct ....

codeworrior

I tested the branch locally by building the Openui5 testsuite and debugging a few simple apps. From a functional point of view, the results look quite good.

However, I have a few comments / questions / suggestions:

some of the fixes you made meanwhile have been made in [FIX] Improve recognition of main module in case of bundles #341 as well. Some need to be removed as they differ from what has been done in [FIX] Improve recognition of main module in case of bundles #341
the added handling of library designtime bundles should be moved to a separate change (1 topic 1 change)
the further optimization of XML views IMO should not be done, see inline comment
building the source maps seems to significantly increase the build time. With the predecessor commit of this branch, a build --clean-dest --all took 3.07 min (avg. over 5 runs), with the sourcemaps it took 4.33 min. I wonder if it is possible (without too complicated code) to skip the sourcemap support and expose that as an option . @RandomByte , @matz3 : what do you think?
another question (which unfortunately makes my lack of understanding transparent :-) ): how essential is the escodegen step for the source map support? Would it continue to work when we provide terser with the AST instead of the code string? My impression is that terser has fixed the issues that uglifyes had with the Spider Monkey AST and newer (ES6) syntax.

codeworrior · 2019-10-02T08:13:57Z

lib/lbt/bundle/Builder.js

@@ -42,7 +43,7 @@ const UI5BundleFormat = {
 		outW.write(`jQuery.sap.registerPreloadedModules(`);
 		outW.writeln(`{`);
 		if ( section.name ) {
-			outW.writeln(`"name":"${section.getSectionName()}",`);
+			outW.writeln(`"name":"${section.name}",`);


Sorry, you might have to rebase again. The same fix has been made in PR #341 where I also tried to close some code coverage gaps un the builder and resolver.

codeworrior · 2019-10-02T08:15:05Z

lib/lbt/bundle/Builder.js

@@ -73,7 +74,7 @@ const EVOBundleFormat = {
 	afterPreloads(outW, section) {
 		outW.write(`}`);
 		if ( section.name ) {
-			outW.write(`,"${section.getSectionName()}"`);
+			outW.write(`,"${section.name}"`);


codeworrior · 2019-10-02T08:16:23Z

lib/lbt/bundle/Builder.js

@@ -288,7 +288,7 @@ class BundleBuilder {
 		const sequence = section.modules.slice();

 		this.beforeWriteFunctionPreloadSection(sequence);
-
+		this.outW.writeln(`//@ui5-bundle ${section.bundle.name}`);


also part of #341 . Not sure if the auto-merge can handle this.

Rebase can :)

codeworrior · 2019-10-02T08:18:56Z

lib/lbt/bundle/Builder.js

+							column: mapping.originalColumn
+						},
+						source: mapping.originalLine != null ?
+							(path.join(path.relative(path.dirname(this._sourceMap._file), path.dirname(resource.name)), this.optimize ? ModuleName.getDebugName(path.basename(resource.name)) : path.basename(resource.name))).replace(/\\/g, "/") : null,


the special handling for \ looks very suspicious to me. AFAIK, we do this nowhere.

And line length won't make @RandomByte happy.

Well i'm not sure how you deal with the path operation so far, but I couldn't find anything specific.
In my case I need to get relative file path for the sources, usually this would be done with path directly but since you have a vfs behind the scene all file paths are unix like (which in turn break the usage of path on windows).
I would love for a better alternative (which doesn't involve rewriting basic functionalities of node)

since you have a vfs behind the scene all file paths are unix like (which in turn break the usage of path on windows)

When working with paths coming from or passed to our vfs, use path.posix. E.g. path.posix.relative.

In cases were you have a native path and want to use it with the vfs, we use the slash module:

ui5-builder/lib/types/application/ApplicationFormatter.js

Lines 41 to 46 in 9d213ce

let projectPath = this._project.path;

if (posix) {

projectPath = slash(projectPath);

p = path.posix;

}

return p.join(projectPath, this._project.resources.pathMappings["/"]);

codeworrior · 2019-10-02T08:22:11Z

lib/lbt/bundle/Builder.js

@@ -440,6 +484,8 @@ class BundleBuilder {
 				// because whitespace of HTML <pre> should be preserved (should only happen rarely)
 				if (!xmlHtmlPrePattern.test(fileContent)) {
 					fileContent = pd.xmlmin(fileContent.toString(), false);
+					fileContent = fileContent.replace(/\n/g, "");


Please remove these two replacement steps. They can't differentiate between whitespace between tags and whitespace inside e.g. string literals in expression bindings. It is hard to imagine how such unescaped whitespace could be used, but not modifying those parts of the view is a matter of principle.

I changed this because it was actually difference with the current tool :) But I can remove it for now

codeworrior · 2019-10-02T08:23:45Z

lib/lbt/bundle/Builder.js

 			// rewrite sap.ui.define to sap.ui.predefine
 			if ( defineCall.callee.type === Syntax.MemberExpression
 					&& defineCall.callee.property.type === Syntax.Identifier
-					&& defineCall.callee.property.name === "define" ) {
+					&& defineCall.callee.property.name === "define") {


unbalanced. I understand that whitespace after/before parentheses is a matter of taste, but mixing both approaches does not qualify as a good compromise, IMHO :-)

one word: prettier

codeworrior · 2019-10-02T08:24:42Z

lib/lbt/bundle/Resolver.js

@@ -253,7 +253,7 @@ class BundleResolver {

 				// NODE-TODO long t0=System.nanoTime();
 				const resolvedSection = resolved.sections[index];
-
+				resolvedSection.name = section.name;


please remove, Now handled differently with PR #341 .

codeworrior · 2019-10-02T08:26:32Z

lib/processors/uglifier.js

@@ -1,5 +1,6 @@
 const terser = require("terser");
 const copyrightCommentsPattern = /copyright|\(c\)(?:[0-9]+|\s+[0-9A-za-z])|released under|license|\u00a9/i;
+// const EvoResource = require("@ui5/fs").Resource;


not needed?

Well, that's the whole problem with the current flow.
The preload files are created before the debug files are created and the source uglified, so in effect some of the job is done twice (uglify) and this also doesn't work well with external transpilation.

Ideally the flow should allow for

src (es6 / yolo)

transpilation (with source map to the original file)

uglify (with source map resolved to the original file)

bundling (with source map resolved to the original files)

The easiest way would be to consider

source

copy to dist as debug files (saved as is)

perform transformation(s) and bundling on the -dbg files

codeworrior · 2019-10-02T08:28:36Z

lib/tasks/bundlers/generateLibraryPreload.js

@@ -48,15 +49,18 @@ function getBundleDefinition(namespace) {
 		};
 	}
 	return {
-		name: `${namespace}/library-preload.js`,
-		defaultFileTypes: [".js", ".fragment.xml", ".view.xml", ".properties", ".json"],
+		name: `${namespace}/library-preload${isDesignTime ? ".designtime" : ""}.js`,


can we please address the handling of designtime bundles in a separate change. It totally makes sense, but I would suggest to follow the principle "1 topic - 1 change".

sure I'll put that on the side

nlunets · 2019-10-03T07:31:22Z

another question (which unfortunately makes my lack of understanding transparent :-) ): how essential is the escodegen step for the source map support? Would it continue to work when we provide terser with the AST instead of the code string? My impression is that terser has fixed the issues that uglifyes had with the Spider Monkey AST and newer (ES6) syntax.

I think (but would need to check), that the overall process (even the bundling) could work on the AST objects directly, avoiding the back and forth from code to AST which may be a little be costly.
I'll make the changes you requested first and extract the design time stuff for now and will look at what we can do there.
However please also take a look at my comment on the task workflow as I think it's also relevant for all this.

nlunets · 2019-10-23T12:36:49Z

Ok, while I fix the tests here are some details.

The code is now parsed once with terser to AST and minification is then done from the AST directly, avoiding the "useless" escodegen in the middle.
No more weird path !

nlunets · 2019-10-23T15:05:11Z

@RandomByte @matz3 I doubt the error on the pipeline are normal, any way to rerun it ?

nlunets · 2019-10-23T15:10:54Z

lib/processors/bundlers/moduleBundler.js

@@ -118,7 +118,7 @@ module.exports = function({resources, options}) {
 	const pool = new LocatorResourcePool();
 	const builder = new BundleBuilder(pool);

-	const bundleOptions = options.bundleOptions || {optimize: true};
+	const bundleOptions = options.bundleOptions || {optimize: true, usePredefineCalls: true};


@codeworrior Am I correct in doing this or did I do too much here ? :)

codeworrior · 2019-12-29T22:26:45Z

Basically, the creation of source maps looks good to me. The change of the order of tasks also makes sense to me, but that's something that I shouldn't decide alone. Next year, we'll do a review together in the team.

One thing that I noticed: only the createDebugFiles task has been moved, but not the uglify task. Was this intentional? As a side effect, uglification breaks the source maps for the bundles and removes the sourceMappingURL annotation. At least for sap-ui-core.js, I could observe this.

One minor thing: I expected some code that modifies the source map for the rewritten sap.ui.define call, but couldn't find anything. Is this handled by omitting the line and column info for the new AST nodes?

nlunets · 2020-01-02T09:12:32Z

One thing that I noticed: only the createDebugFiles task has been moved, but not the uglify task. Was this intentional?

Probably not... although I would except uglification to ignore bundles

One minor thing: I expected some code that modifies the source map for the rewritten sap.ui.define call, but couldn't find anything. Is this handled by omitting the line and column info for the new AST nodes?

Source map voodoo, not needed to do anything

- Always go for the predefine rewrite in the library build - Change the order of the steps to have debug file created early enough - Sourcemap support for the rewrite and for the preload generation - Uglifier can take input sourcemap as well (if you have a custom transpile step...) (cherry picked from commit 4981289)

RandomByte · 2021-02-16T10:27:14Z

fyi: We looked into this PR in more detail and also did a rebase in a separate sourcemap branch.

There have been changes to Terser in v5 and we might change other parts as well. Please don't put any work into rebasing or extending this PR yourself right now. We'll probably replace this PR with a new one once it's planned in our backlog. Thanks again for the work that went into this PR, it really helped us getting started with the topic.

Inspired by #282 and https://github.com/SAP/ui5-builder/compare/sourcemap

Resolves SAP/ui5-tooling#472 Supersedes #282 Based on SAP/ui5-tooling#583 JIRA: CPOUI5FOUNDATION-434 Co-authored-by: Matthias Osswald <mat.osswald@sap.com>

RandomByte · 2022-04-15T09:31:49Z

Superseded by #695

We took over quite some code from this PR for the development of the Source Map support in UI5 Tooling 3.0. Thanks!

RandomByte requested a review from devtomtom July 4, 2019 14:20

RandomByte added the enhancement New feature or request label Jul 4, 2019

RandomByte added this to In progress in UI5 Tooling - Tasks via automation Jul 4, 2019

codeworrior reviewed Jul 4, 2019

View reviewed changes

nlunets force-pushed the sourcemap branch from 7b45950 to 97e3e5b Compare September 10, 2019 12:15

nlunets force-pushed the sourcemap branch 3 times, most recently from 3c637e1 to 6842523 Compare September 20, 2019 13:12

codeworrior requested changes Oct 2, 2019

View reviewed changes

UI5 Tooling - Tasks automation moved this from In progress to Needs review Oct 2, 2019

nlunets force-pushed the sourcemap branch from 6842523 to 39dc97f Compare October 9, 2019 07:19

nlunets force-pushed the sourcemap branch from 9cc6581 to e14bb5e Compare October 23, 2019 12:25

nlunets commented Oct 23, 2019

View reviewed changes

nlunets force-pushed the sourcemap branch from ba8b7b0 to 54edf28 Compare November 12, 2019 12:50

codeworrior requested review from codeworrior and removed request for devtomtom December 29, 2019 22:27

nlunets force-pushed the sourcemap branch from 54edf28 to 57cc113 Compare February 7, 2020 15:53

nlunets force-pushed the sourcemap branch from 57cc113 to 8a874cb Compare February 12, 2020 15:10

nlunets added 13 commits October 5, 2020 11:51

- Test should now all run properly

b213c7e

- Also include component preload map

3a33f18

- lint issue

2e7b4af

- rebase + test changes due to new uglifier

4ca62b8

- change to bundler

35ec208

- fix tests

e845215

-fix on windows probably

5f83b45

Reverts some parts which can be too disruptive for now

39517d1

More reverts

f34d0f2

Leverage the terser AST to avoid back and forth with escodegen

bc18f38

Fix tests

f92da75

Fix source file path

087a4b9

nlunets force-pushed the sourcemap branch from 8a874cb to 087a4b9 Compare October 5, 2020 09:59

nlunets added 2 commits October 5, 2020 13:01

update pacakge-lock

658e38e

fix eslint

42dcc85

RandomByte mentioned this pull request Dec 15, 2020

ui5 cli does not create source maps SAP/ui5-tooling#472

Closed

RandomByte added a commit that referenced this pull request Oct 29, 2021

lbt/bundleBuilder: Add source map transformation

dcdcd39

Inspired by #282 and https://github.com/SAP/ui5-builder/compare/sourcemap

RandomByte added a commit that referenced this pull request Nov 2, 2021

lbt/bundle/Builder: Add source map transformation

b030373

Inspired by #282 and https://github.com/SAP/ui5-builder/compare/sourcemap

RandomByte mentioned this pull request Feb 23, 2022

[FEATURE] Generate source maps for bundles #695

Merged

RandomByte added a commit that referenced this pull request Feb 23, 2022

[FEATURE] Generate source maps for bundles (#695)

8a20c42

Resolves SAP/ui5-tooling#472 Supersedes #282 Based on SAP/ui5-tooling#583 JIRA: CPOUI5FOUNDATION-434 Co-authored-by: Matthias Osswald <mat.osswald@sap.com>

RandomByte closed this Apr 15, 2022

UI5 Tooling - Tasks automation moved this from Needs review to Done Apr 15, 2022

	let projectPath = this._project.path;
	if (posix) {
	projectPath = slash(projectPath);
	p = path.posix;
	}
	return p.join(projectPath, this._project.resources.pathMappings["/"]);

[FEATURE] Sourcemap support in the ui5-builder preload generation #282

[FEATURE] Sourcemap support in the ui5-builder preload generation #282

Conversation

nlunets commented Jul 4, 2019

Pull Request Checklist

CLAassistant commented Jul 4, 2019 • edited Loading

codeworrior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlunets commented Jul 4, 2019

codeworrior commented Jul 5, 2019

nlunets commented Jul 5, 2019

codeworrior commented Jul 5, 2019

nlunets commented Jul 9, 2019

coveralls commented Jul 9, 2019 • edited Loading

nlunets commented Sep 10, 2019

codeworrior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlunets commented Oct 3, 2019

nlunets commented Oct 23, 2019

nlunets commented Oct 23, 2019

Choose a reason for hiding this comment

codeworrior commented Dec 29, 2019

nlunets commented Jan 2, 2020

RandomByte commented Feb 16, 2021

RandomByte commented Apr 15, 2022

CLAassistant commented Jul 4, 2019 •

edited

Loading

coveralls commented Jul 9, 2019 •

edited

Loading