-
Notifications
You must be signed in to change notification settings - Fork 23
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] Sourcemap support in the ui5-builder preload generation #282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you avoid writing out the *.map for the minified individual files? I missed that part...
lib/lbt/bundle/BundleWriter.js
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're correct, it works today because I just deal with minified, one line files ;)
lib/lbt/bundle/Builder.js
Outdated
// 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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move back to original position , please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you had prettier we wouldn't have this conversation ! (just teasing, yes I'll move it back)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So 'prettier' basically is a tool that keeps people from talking to each other, did I get that right?
I do not avoid creating the map for the individual minified files, I actually want that, but that whole part is weird anyway. |
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:
... |
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 ? |
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. |
I've removed the automatic creation of map for the uglified files now and the test should all run. |
@codeworrior This has been rebased and should be fine for review "code wise". |
3c637e1
to
6842523
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/lbt/bundle/Builder.js
Outdated
@@ -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}",`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
lib/lbt/bundle/Builder.js
Outdated
@@ -73,7 +74,7 @@ const EVOBundleFormat = { | |||
afterPreloads(outW, section) { | |||
outW.write(`}`); | |||
if ( section.name ) { | |||
outW.write(`,"${section.getSectionName()}"`); | |||
outW.write(`,"${section.name}"`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
lib/lbt/bundle/Builder.js
Outdated
@@ -288,7 +288,7 @@ class BundleBuilder { | |||
const sequence = section.modules.slice(); | |||
|
|||
this.beforeWriteFunctionPreloadSection(sequence); | |||
|
|||
this.outW.writeln(`//@ui5-bundle ${section.bundle.name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also part of #341 . Not sure if the auto-merge can handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase can :)
lib/lbt/bundle/Builder.js
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the special handling for \ looks very suspicious to me. AFAIK, we do this nowhere.
And line length won't make @RandomByte happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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["/"]); |
lib/lbt/bundle/Builder.js
Outdated
@@ -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, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this because it was actually difference with the current tool :) But I can remove it for now
lib/lbt/bundle/Builder.js
Outdated
// 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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one word: prettier
lib/lbt/bundle/Resolver.js
Outdated
@@ -253,7 +253,7 @@ class BundleResolver { | |||
|
|||
// NODE-TODO long t0=System.nanoTime(); | |||
const resolvedSection = resolved.sections[index]; | |||
|
|||
resolvedSection.name = section.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove, Now handled differently with PR #341 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I'll put that on the side
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. |
Ok, while I fix the tests here are some details.
|
@RandomByte @matz3 I doubt the error on the pipeline are normal, any way to rerun it ? |
@@ -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}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeworrior Am I correct in doing this or did I do too much here ? :)
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 |
Probably not... although I would except uglification to ignore bundles
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)
fyi: We looked into this PR in more detail and also did a rebase in a separate 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. |
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>
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! |
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