W-18988586 feat: support paths for ConfigAggregator #1211
Conversation
| @@ -116,16 +116,27 @@ export class ConfigAggregator extends AsyncOptionalCreatable<ConfigAggregator.Op | |||
|
|
|||
| // Use typing from AsyncOptionalCreatable to support extending ConfigAggregator. | |||
| // We really don't want ConfigAggregator extended but typescript doesn't support a final. | |||
| public static async create<T extends ConfigAggregator>( | |||
There was a problem hiding this comment.
this makes the types cleaner inside the method, but is kinda ugly
explanation: because this could be subclassed, the original let anything that extends AsyncOptionalCreatable use it.
we can have a tighter condition , where if we know the thing is really a ConfigAggregator (or its subclasses) the types can be more specific.
I'd also be happy to let create only work with ConfigAggregator but I don't understand heavy-OOP to know the implications. There are a few extends ConfigAggregator I saw in the wild on github so I wanted to be careful with the public API.
src/config/configAggregator.ts
Outdated
| ConfigAggregator.instance = new this(); | ||
| (ConfigAggregator.instance as ConfigAggregator).loadPropertiesSync(); | ||
| private static getInstance(projectPath = process.cwd()): ConfigAggregator { | ||
| if (!ConfigAggregator.instances.has(projectPath)) { |
There was a problem hiding this comment.
I think this has the potential to be sketchy, even before this PR.
because getValue is sync, we can't do the real create here. So I think it's going to have a ConfigAggregator missing the options.configMeta stuff?
There was a problem hiding this comment.
Agreed - but I think that's why the jsdoc points out that you should call reload after
There was a problem hiding this comment.
I'll try explaining better:
getValue calls getInstance.
so if you say getValue and pass it something that you'd expect to be loaded from custom configMeta, it's not.
Because there's no way to pass options in via getValue like you would if you were using async create.
|
QA: 🟢 🟡 It's not scoping the ConfigAggregator to the project - or am I misunderstanding the intent of the PR? Here's the script import { ConfigAggregator } from "@salesforce/core";
const configAgg1 = await ConfigAggregator.create();
console.log(`Config Aggregator 1: ${process.cwd()}`);
console.log(configAgg1.getConfig());
const configAgg2 = await ConfigAggregator.create({projectPath: "~/repos/VivekMChawla/afdx-test/"});
console.log(`Config Aggregator 2: ~/repos/VivekMChawla/afdx-test/`);
console.log(configAgg2.getConfig());
|
What does this PR do?
ConfigAggregator is a singleton
instead of caching 1 ConfigAggregator instance, it has one per sfdx project
to help with multiple project and VSCode when the user changes projects and the singleton has to be destroyed and recreated.
What issues does this PR fix or reference?
@W-18988586@