feat: PoC: node modules & webcomponents integration by d3xter666 · Pull Request #1287 · UI5/cli
Conversation
|
|
||
| - **UI5 Application Source** — The starting point is the developer's application code. Controllers reference NPM packages via AMD dependencies (e.g., `sap.ui.define(['thirdparty/chart.js', ...])`), and XML views reference them via xmlns declarations (e.g., `xmlns:webc="thirdparty.@ui5.webcomponents"`). These `thirdparty/*` references are the convention that triggers the NPM integration pipeline. | ||
|
|
||
| - **Phase 1: Integrated Dependency Analysis** — The existing `JSModuleAnalyzer` and `XMLTemplateAnalyzer` are extended to detect `thirdparty/*` patterns during their regular AST traversal. `espree.parse` walks the AST looking for `thirdparty/*` in AMD dependency arrays and ESM import statements, while `XMLTemplateAnalyzer` scans for `xmlns:thirdparty.*` declarations. `ResourceCollector` aggregates all detected NPM package names across all analyzed resources. **Key architectural decision**: This piggybacks on the AST parsing that already occurs for UI5 dependency analysis — no separate scanning phase is needed, eliminating duplicate parsing overhead. Output: a deduplicated set of NPM package names (e.g., `['chart.js', 'react', 'react-dom', '@ui5/webcomponents']`). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This thirdparty/* naming convention is only introduced in this concept, right? ui5-tooling-modules seems to simply use the npm package name. Why do we need the prefix here?
Wouldn't this break TypeScript use cases where an import for react would give you the corresponding types but thirdparty/react wouldn't resolve them anymore?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting point and I fully agree with it, but yes, it's kind of "new" concept here.
Here are my toughts on this:
- If we have a direct package name being referenced directlty i.e.
react, this means that this package is not UI5 AMD wrapped and the code that follows knows how to handle the raw format. The package is taken directly fromnode_mdules/ - Our current situation (in UI5 repositories) is that all the packages/thirdparties that are (usually) UI5 AMD wrapped and stay in a corresponding
thirdparty/folder.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored the document to try to explain my points clearly
|
|
||
| - **Phase 1: Integrated Dependency Analysis** — The existing `JSModuleAnalyzer` and `XMLTemplateAnalyzer` are extended to detect `thirdparty/*` patterns during their regular AST traversal. `espree.parse` walks the AST looking for `thirdparty/*` in AMD dependency arrays and ESM import statements, while `XMLTemplateAnalyzer` scans for `xmlns:thirdparty.*` declarations. `ResourceCollector` aggregates all detected NPM package names across all analyzed resources. **Key architectural decision**: This piggybacks on the AST parsing that already occurs for UI5 dependency analysis — no separate scanning phase is needed, eliminating duplicate parsing overhead. Output: a deduplicated set of NPM package names (e.g., `['chart.js', 'react', 'react-dom', '@ui5/webcomponents']`). | ||
|
|
||
| - **Transitive Dependency Consolidation** — Before Rollup runs, each scanned package's full transitive dependency tree is enumerated by walking `package.json` `dependencies` fields recursively. Any transitive dependency appearing in two or more trees must be externalized — bundled as its own standalone AMD module rather than being inlined into each consumer's bundle. This produces an externals map (which deps to exclude per package) and may add newly-discovered shared packages to the bundle set (see [Section 3.8.3](#383-externals-discovery-via-transitive-dependency-consolidation)). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
I'll take a look at it. As you can see in the PoC, currently we have the "external" (transitive) dependencies being hardocded as I wanted to try the feasibility of the build and the expected output
|
|
||
| #### 3.4.2 The `thirdparty/*` Convention | ||
|
|
||
| The expected namespace convention for NPM packages in UI5 code: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above, why are we introducing this convention?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a lot easier if we have a concept like this one.
Otherwise, we must do some kind of heuristics in order to identify external packages which might be fragile and hard to implement.
#1287 (comment)
|
|
||
| #### 3.7.3 Advanced Edge Cases | ||
|
|
||
| These edge cases are handled based on `ui5-tooling-modules` production experience: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This describes what ui5-tooling-modules provides. We should also document which of these we need to implement in UI5 CLI as well
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the idea- to discuss what we can and want to deliver
|
|
||
| #### 3.8.1 Design Principle | ||
|
|
||
| **Core concept:** One NPM package = One AMD module file. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this. Won't rollup in some cases bundle multiple npm packages into one AMD module that is then imported in the app?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I need to be more clear here. It's more like:
one entry point (import/require/sap.ui.define) in UI5 source files === npm package
But this does not mean that the npm package is standalone. The build is handled by rollup and It can be bundled along with its transitive dependencies.
|
|
||
| The externals + paths mechanism solves this: `react-dom` and `react-colorful` declare `react` as external, and their AMD output references `thirdparty/react` instead of inlining React's code. | ||
|
|
||
| #### 3.8.3 Externals Discovery via Transitive Dependency Consolidation |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is my understanding correct that this will also define the entry points for rollup?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
This is the idea of deduplication of (transitive dependencies) packages that will be resued by other packages.
Those transitive dependencies won't be used directly in UI5's app source code, but by thirdparties, dependending on them
|
|
||
| #### 3.8.4 Build Order Optimization | ||
|
|
||
| At **build time**, packages must be bundled in topological order (dependencies first) so that externals are resolved correctly: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we need to be aware of? I would think this is handled within rollup.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunatelly, not!
As far as I got this, we must first build the "clean" dependencies, so that they can be reused (as imports) for the rest of bundling. Otherwise, rollup might:
- not discover them correctly
- bundle everything
|
|
||
| #### 3.10.1 Motivation | ||
|
|
||
| NPM packages sometimes contain bugs, security vulnerabilities, or incompatibilities that the upstream maintainer has not yet fixed — or cannot fix in a timely manner. In these cases, the consuming project must apply a targeted fix (patch) to the package source *before* Rollup bundles it. Without a patching mechanism, the only alternatives are forking the package or waiting for an upstream release. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should discuss whether patching should be part of the initial release of this feature. I'm still uncomfortable adding such functionality while most other web frameworks do not provide something similar.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must definitelly discuss this! It's just an idea as we have a lot of packges being patched currently.
However, when we have the possibility to update and monitor packages quickly via package.json, the idea of patching might become redundant.
WebComponents currently do that, for example: https://github.com/UI5/webcomponents/tree/main/.yarn/patches
They rely on yarn, though!
This file contains hidden or 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