-
Notifications
You must be signed in to change notification settings - Fork 72
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
[RFC] 00010 UI5 Builder-Bundling Refactoring #583
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
43ebf2e
[RFC] 00010 UI5 Builder-Bundling Refactoring
RandomByte cfe2c1a
Expand on new minify task
RandomByte 21682b6
Add diagrams
RandomByte ba11c47
Outline documentation needs
RandomByte 6301944
Reorder chapters
RandomByte d49a6c4
Fix indent
RandomByte 6329f7a
Add open question: Should bundle/Builder also generate source maps?
RandomByte 5c84b66
Add notes from discussion regarding resourceRoot-mappings and static …
RandomByte 6c6f066
Simplify ReaderFilter: Use a filter callback
RandomByte c8d1a0d
Update based on implementation
RandomByte c6d5754
Typo
matz3 05b6a5c
Update section of handling -dbg resources according to new concept
matz3 df1e705
Update concept based on existing implementation
matz3 b480bab
Further updates
matz3 c497ea7
Add drawbacks
matz3 5447038
Remove comments / empty sections
matz3 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
- Start Date: 2021-11-09 | ||
- RFC PR: [#583](https://github.com/SAP/ui5-tooling/pull/583) | ||
- Issue: - | ||
- Affected components <!-- Check affected components by writing an "X" into the brackets --> | ||
+ [x] [ui5-builder](https://github.com/SAP/ui5-builder) | ||
+ [ ] [ui5-server](https://github.com/SAP/ui5-server) | ||
+ [ ] [ui5-cli](https://github.com/SAP/ui5-cli) | ||
+ [x] [ui5-fs](https://github.com/SAP/ui5-fs) | ||
+ [ ] [ui5-project](https://github.com/SAP/ui5-project) | ||
+ [ ] [ui5-logger](https://github.com/SAP/ui5-logger) | ||
|
||
|
||
# RFC 0010 UI5 Builder-Bundling Refactoring | ||
|
||
## Summary | ||
|
||
Refactor the bundle creation process of UI5 Tooling to make it more efficient and to provide new features. | ||
|
||
## Motivation | ||
|
||
The current bundling process is lacking in multiple aspects. The main pain points are as follows: | ||
|
||
- *Efficiency:* **Minification** of resources is done as part of the bundling process, although the same resources are also minified in a following "uglify" task as part of the overall build process | ||
- *Feature:* **Source maps** are neither generated nor are existing source maps used in a bundle resource | ||
- *Issue:* The bundle process can't **differentiate between resources and their debug-variants** (i.e. `-dbg.js` files). In some cases this can lead to both resources being included in a bundle. | ||
|
||
## Detailed design | ||
|
||
### Correct handling of debug-files | ||
|
||
Depending on whether the requested bundle shall be "optimized", filter out all debug- or non-debug variants of resources. This is achieved by relying on resources being tagged with either "IsDebugVariant" or "HasDebugVariant", which is done by the task creating the debug variant. | ||
|
||
These tags are shared in a "global" build context, so that they can also be accessed when working with resources of a dependency that has been built earlier. | ||
|
||
#### `@ui5/fs.readers.Filter` | ||
|
||
Filtering resources based on a tag can be achieved by enhancing on the [ReaderCollection](https://sap.github.io/ui5-tooling/api/module-@ui5_fs.ReaderCollection.html) concept of the UI5 FS. A new `filter()` API of the [AbstractReader](https://sap.github.io/ui5-tooling/api/module-@ui5_fs.AbstractReader.html) will provide a "reader.Filter" instance *(which also implements `AbstractReader`)*, wrapping the given reader instance. For every resource retrieved through this reader.Filter, a filter callback function is called, allowing filtering on tags or other parameters. | ||
|
||
This approach of filtering-out resources that should never be included in a specific bundle can also be used when exchanging the legacy-bundle-tooling with other tools like [Rollup](https://github.com/rollup/rollup), which also only expect the resources relevant for the bundle to be available. | ||
|
||
**Signature:** | ||
```javascript | ||
/** | ||
* Filter constructor | ||
* | ||
* @param {object} parameters Parameters | ||
* @param {module:@ui5/fs.AbstractReader} parameters.reader A resource reader | ||
* @param {Function} parameters.filterCallback | ||
* Filter function. Should return true for items to keep and false otherwise | ||
*/ | ||
|
||
byPath(...) | ||
byGlob(...) | ||
``` | ||
|
||
**Example:** | ||
```javascript | ||
const filteredWorkspace = workspace.filter(function(resource) { | ||
return !taskUtil.getTag(resource, taskUtil.STANDARD_TAGS.IsDebugVariant); | ||
}); | ||
|
||
const resources = await filteredWorkspace.byGlob("**"); // Won't return any resources tagged as "IsDebugVariant" | ||
``` | ||
|
||
#### `@ui5/builder.processors.moduleBundler` | ||
|
||
Bundles can also be configured as `optimize: false`. This should create a bundle consisting of files that have not been minified. One can invert the above Filter logic to exclude any minified files that have a debug variant (tagged as `HasDebugVariant`). However the remaining files will have the suffix `-dbg` (for example the unminified resource for `MyControl.js` is named `MyControl-dbg.js`). Depending on the bundle configuration, this naming difference can cause resources to incorrectly be in- or excluded from the bundle. | ||
|
||
To solve this, a new option `moduleNameMapping` is introduced to the `moduleBundler` processor. | ||
The mapping allows the processor to use the given module name instead of deriving the module name from the resource path. | ||
|
||
**Example:** | ||
```javascript | ||
|
||
const bundles = await moduleBundler({ | ||
options: { | ||
bundleDefinition, | ||
bundleOptions, | ||
moduleNameMapping: { | ||
"/resources/MyControl-dbg.js": "MyControl.js" | ||
} | ||
}, | ||
resources // Contains resource with path /resources/MyControl-dbg.js | ||
}); | ||
``` | ||
|
||
### Reorder- and reorganize build tasks | ||
To make the above possible, the minification (a.k.a. "uglification") as well as the debug-file creation tasks need to be done before the bundling process. Only this allows the bundling to make use of the minified resources and the accompanying source maps. | ||
|
||
**Create a new "minify" task** which takes care of creating a debug-variant, minifying the original resource (and creating a source map in the process) as well as tagging the resources as "IsDebugVariant" and "HasDebugVariant" accordingly. | ||
|
||
**Remove the existing "uglify" and "createDebugFiles" tasks** in favor of this new task. Throw an error when those tasks are used, e.g. in a "beforeTask" or "afterTask" configuration. | ||
|
||
**"minify" Task Activity:** | ||
|
||
![minify Task Activity](./resources/UI5_Builder-Bundling_Refactoring_-_minify_task.png) | ||
|
||
|
||
### Source Map support | ||
|
||
For every JavaScript resource that is being bundled, search for a "sourceMappingURL"-tag at the end of the file and try to load the referenced source map. For source maps located in separate files, this could look like `//# sourceMappingURL=DateFormat.js.map`. For inline source maps this might look like `//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ3XZJka...`. | ||
|
||
The outdated annotation convention `//@` should not be supported. All known relevant tools (Terser, TypeScript compiler) are using the new convention. See [Conventions](http://sourcemaps.info/spec.html#h.lmz475t4mvbx) in the source map specification for details. | ||
|
||
Any source maps found for a resource shall be added to the source map of the bundle. Either use an [index map](http://sourcemaps.info/spec.html#h.535es3xeprgt), containing the individual source maps, or generate a new source map for the whole bundle based of them. | ||
|
||
In case bundled sources are modified (e.g. by adding characters), the relevant source map needs to be aligned with that change. | ||
|
||
For resources where no source map is provided, the bundling process should generate one by mapping the content line by line. | ||
|
||
A "transient" source map needs to be generated for any statically generated "require"/"try-catch" code at the end of a bundle. The mapped source needs to be defined in the "sourcesContent" field of the source map. | ||
|
||
**Source Map Handling Activity:** | ||
|
||
![Source Map Handling](./resources/UI5_Builder-Bundling_Refactoring_-_Source_Map_Handling.png) | ||
|
||
## How we teach this | ||
|
||
The new "minify" tasks and the removal of the old "uglify" and "createDebugFiles" tasks should be documented in the ["Standard Tasks"](https://sap.github.io/ui5-tooling/pages/Builder/#standard-tasks) chapter of the Builder documentation. | ||
|
||
Source map support should be documented in a separate chapter in the [Builder](https://sap.github.io/ui5-tooling/pages/Builder/) documentation. Especially with regards to further integration scenarios like projects that have already done a transpilation process (e.g. from TypeScript to JavaScript) and would like to provide us with source maps generated in the process. | ||
|
||
The new `ReaderFilter` API should be documented in JSDoc. The signatures of the existing `generateBundle` tasks (including `generateComponentPreload` and `generateLibraryPreload`) should ideally stay the same, only adding an **optional** `taskUtil` parameter for filtering based on the mentioned tags. This allows for continued easy direct consumption of the tasks. | ||
|
||
## Drawbacks | ||
|
||
### Missing minification for resources from dependencies | ||
|
||
There is a gap in the minification process which causes a regression for some scenarios. | ||
JavaScript resources from other projects (dependencies) are not minified when not building the project with the `--all` flag. | ||
This is because the bundling task does not perform the minification but rather relies on this to be done by the `minify` task. | ||
|
||
Requiring to always build a project with `--all` is not favorable due to performance reasons. Therefore, a solution is needed to still perform the needed minification steps for the mentioned resources. | ||
|
||
### resourceRoot-mappings | ||
|
||
When bundling modules via resourceRoot-mappings, it might become impossible to reference the original source from a bundle's source map. For example, for a bundle located in `/test-resources`, which includes modules from `/resources`, the source map must not reference the original module via `../../resources/x/y/z` as this path rather depends on the resourceRoot-mapping and might be different at runtime. | ||
|
||
This could still be solved at a later time, once resourceRoot-mappings are actually supported by the general bundling process. | ||
|
||
The actual impact of this behavior is uncritical, since it only causes affected source maps to not be found at runtime. The bundle itself as well as other source maps stay consistent. | ||
|
||
## Alternatives | ||
|
||
## Unresolved Questions and Bikeshedding |
Binary file added
BIN
+25.6 KB
rfcs/resources/UI5_Builder-Bundling_Refactoring_-_Source_Map_Handling.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
LGTM.
Line 172 IMO should be moved into the "Source Map support" section, too.
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.
Done