refactor: Consolidate UI5 data directory resolution into resolveUi5DataDir#1456
refactor: Consolidate UI5 data directory resolution into resolveUi5DataDir#1456d3xter666 wants to merge 15 commits into
Conversation
…taDir Introduces packages/project/lib/utils/dataDir.js with a single resolveUi5DataDir() utility that encapsulates the full resolution chain: 1. UI5_DATA_DIR environment variable 2. ui5DataDir from configuration file (~/.ui5rc) 3. Default: ~/.ui5 Removes the duplicate getUi5DataDir() function from packages/cli/lib/framework/utils.js and migrates its callers (createFrameworkResolverInstance, frameworkResolverResolveVersion) to use resolveUi5DataDir() directly. The defensive fallbacks in the public Resolver/Installer APIs (AbstractResolver, Openui5Resolver, Sapui5Resolver, Sapui5MavenSnapshotResolver) are intentionally kept — those are synchronous constructors that cannot call the async utility, and they are publicly exported so external callers may pass undefined.
…ve path resolution Renames the cwd option to projectRootPath to make its purpose explicit: this is the root of the specific project being processed (where package.json lives), not necessarily the shell's current working directory. When omitted, falls back to process.cwd(). Callers in createFrameworkResolverInstance and frameworkResolverResolveVersion now thread the project root through so relative ui5DataDir values in config are resolved against the correct base directory.
…i5DataDir
Two call sites that duplicated the env→config→default resolution logic
are migrated to resolveUi5DataDir({projectRootPath}):
- packages/project/lib/graph/helpers/ui5Framework.js:
enrichProjectGraph resolves ui5DataDir via resolveUi5DataDir using
rootProject.getRootPath() as projectRootPath, then passes it to
the framework Resolver. Configuration and os imports removed.
- packages/project/lib/build/cache/CacheManager.create():
The no-ui5DataDir path now delegates to resolveUi5DataDir instead of
inlining env/config/default logic. The explicit ui5DataDir option
still resolves against cwd for backward compatibility. os and
Configuration imports removed.
Tests updated: resolveUi5DataDir stub added to ui5Framework.js esmocks
so the stub mirrors the real resolution logic (including env and config
stubs), and package-exports count updated.
…i5DataDir
Two call sites that duplicated the env→config→default resolution logic
are migrated to resolveUi5DataDir({projectRootPath}):
- packages/project/lib/graph/helpers/ui5Framework.js:
enrichProjectGraph resolves ui5DataDir via resolveUi5DataDir using
rootProject.getRootPath() as projectRootPath, then passes it to
the framework Resolver. Configuration and os imports removed.
- packages/project/lib/build/cache/CacheManager.create():
The no-ui5DataDir path now delegates to resolveUi5DataDir instead of
inlining env/config/default logic. The explicit ui5DataDir option
still resolves against cwd for backward compatibility. os and
Configuration imports removed.
Tests updated: resolveUi5DataDir stub added to ui5Framework.js esmocks
so the stub mirrors the real resolution logic (including env and config
stubs), and package-exports count updated.
95e6631 to
b449a91
Compare
Replace hardcoded POSIX absolute paths (/my/project, /custom/data/dir, etc.) with path.resolve(os.tmpdir(), ...) so assertions hold on all platforms. On Windows, /foo is not an absolute path — it is relative to the current drive root — causing four tests to fail.
b449a91 to
b091c77
Compare
- Use path.resolve instead of path.join for the default ~/.ui5 fallback so the result is absolute even when os.homedir() returns a relative path (e.g. "./" in some CI/container environments). - Add edge-case test that asserts absoluteness when HOME is relative. - Fix CLI framework/utils tests: stubs resolving undefined reflected old behavior where no ui5DataDir meant undefined; add clarifying comments that these tests validate stub wiring, not real resolution. - Add utils/dataDir to the explicit API parity list in package-exports test so a path/mapping regression would be caught by a specific test.
- cli/framework/utils.js: stub now resolves the real default path (path.join(os.homedir(), ".ui5")) instead of undefined, matching the production contract of resolveUi5DataDir. - graph/helpers/ui5Framework.js: stub and assertions use path.resolve instead of path.join for the default ~/.ui5 path, matching the stricter absolute-path guarantee of the implementation.
The stub for resolveUi5DataDir does not need to compute a real filesystem path — it just needs an opaque value that flows through correctly. Replace path.join(os.homedir(), '.ui5') with the string literal 'my-default-ui5-data-dir' and remove the unused os and path imports.
|
I'm wondering whether we still need the fallbacks in https://github.com/UI5/cli/blob/refactor/resolve-ui5-data-dir/packages/project/lib/ui5Framework/AbstractResolver.js#L44 and https://github.com/UI5/cli/blob/refactor/resolve-ui5-data-dir/packages/project/lib/build/cache/CacheManager.js#L77 since we now always provide a |
I had the same concnerns! Removing the |
|
I'm arguing to make it a required argument for AbstractResolver. The current state in this PR is confusing IMHO: Passing |
Thank you @RandomByte ! I have tried to analyze the situation and how ui5DataDir evolves in the chain. CLIFrom CLI perspective, we have encapsulation and all the resolutions come from the newly introduced APIThe API perspective is a bit more complex and I will try to simplify it by couple of API calls to illustrate it clearer. Resolver.resolveVersion({verions: 1.xxxx}) // ui5DataDir === undefined -> ~/.ui5
// resolveUi5DataDir() -> eventually can fallback to ~/.ui5, but before that will
// consider UI5_DATA_DIR and ui5.yaml configuration.
CacheManager.create(); // ui5DataDir === undefinedIt is obvious that for the same default values for With that refactoring, Here's the resolution: 9ac64b5 |
085bae7 to
9ac64b5
Compare
|
I'm sorry, but I think this doesn't address my concern. There are still two public APIs that resolve the data dir differently. It seems like you just moved the resolution from one internal class to another, which doesn't change the behavior for external consumers:
Take note that As I said before, I think aligning this thoroughly might mean that we need to force consumers of all public APIs to always provide the data dir, which no internal fallback. This might be a bigger refactoring, which we might not want to tackle now. |
Introduces
packages/project/lib/utils/dataDir.jswith a singleresolveUi5DataDir()utility that encapsulates the full resolution chain:UI5_DATA_DIRenvironment variableui5DataDirfrom configuration file~/.ui5Removes the duplicate
getUi5DataDir()function frompackages/cli/lib/framework/utils.jsand migrates its callers (createFrameworkResolverInstance,frameworkResolverResolveVersion) to useresolveUi5DataDir()directly.Consolidates and enforces the new utility accross packages