Kill kbn_observable and @kbn/observable#21944
Kill kbn_observable and @kbn/observable#21944spalger merged 10 commits intoelastic:masterfrom rhoboat:rm-kbn-observable
Conversation
|
Still WIP until CI green |
|
|
||
| // Used to indicate that no config has been received yet | ||
| const notRead = Symbol('config not yet read'); | ||
| const notRead: symbol = Symbol('config not yet read'); |
There was a problem hiding this comment.
question: why do need this change? The type should be inferred.
There was a problem hiding this comment.
unique symbol is not assignable to any...
| * every new subscription will immediately receive the current config. | ||
| */ | ||
| private readonly rawConfigFromFile$: BehaviorSubject<any> = new BehaviorSubject(notRead); | ||
| private readonly rawConfigFromFile$: BehaviorSubject<symbol> = new BehaviorSubject(notRead); |
There was a problem hiding this comment.
issue: The very first value will be symbol, but consequent values will be config's.
There was a problem hiding this comment.
I've tried a couple of things to type this differently, but i think the problem is any does not cover the Symbol type.
here's one of the things i tried, but there's an error with type when we call next on the behavior subject.
import { isEqual, isPlainObject } from 'lodash';
import { BehaviorSubject, from, Observable } from 'rxjs';
import { filter, map, skipWhile } from 'rxjs/operators';
import typeDetect from 'type-detect';
import { ObjectToRawConfigAdapter } from './object_to_raw_config_adapter';
import { RawConfig } from './raw_config';
import { getConfigFromFile } from './read_config';
// Used to indicate that no config has been received yet
const notRead: symbol = Symbol('config not yet read');
export class RawConfigService {
/**
* The stream of configs read from the config file. Will be the symbol
* `notRead` before the config is initially read, and after that it can
* potentially be `null` for an empty yaml file.
*
* This is the _raw_ config before any overrides are applied.
*
* As we have a notion of a _current_ config we rely on a BehaviorSubject so
* every new subscription will immediately receive the current config.
*/
private readonly rawConfigFromFile$:
| BehaviorSubject<symbol>
| BehaviorSubject<any> = new BehaviorSubject(notRead);
private readonly config$: Observable<RawConfig>;
constructor(readonly configFile: string) {
this.config$ = from(this.rawConfigFromFile$).pipe(
filter(rawConfig => rawConfig !== notRead),
map(rawConfig => {
// If the raw config is null, e.g. if empty config file, we default to
// an empty config
if (rawConfig == null) {
return new ObjectToRawConfigAdapter({});
}
if (isPlainObject(rawConfig)) {
// TODO Make config consistent, e.g. handle dots in keys
return new ObjectToRawConfigAdapter(rawConfig);
}
throw new Error(`the raw config must be an object, got [${typeDetect(rawConfig)}]`);
}),
// We only want to update the config if there are changes to it
skipWhile(isEqual)
);
}
/**
* Read the initial Kibana config.
*/
public loadConfig() {
const config = getConfigFromFile(this.configFile);
this.rawConfigFromFile$.next(config); // Cannot invoke an expression whose type lacks a call signature. Type '((value: symbol) => void) | ((value: any) => void)' has no compatible call signatures.
}
public stop() {
this.rawConfigFromFile$.complete();
}
/**
* Re-read the Kibana config.
*/
public reloadConfig() {
this.loadConfig();
}
public getConfig$() {
return this.config$;
}
}There was a problem hiding this comment.
Here's another way, remove the symbol and behavior subject type annotations and there's a similar error on the same line
import { isEqual, isPlainObject } from 'lodash';
import { BehaviorSubject, from, Observable } from 'rxjs';
import { filter, map, skipWhile } from 'rxjs/operators';
import typeDetect from 'type-detect';
import { ObjectToRawConfigAdapter } from './object_to_raw_config_adapter';
import { RawConfig } from './raw_config';
import { getConfigFromFile } from './read_config';
// Used to indicate that no config has been received yet
const notRead = Symbol('config not yet read');
export class RawConfigService {
/**
* The stream of configs read from the config file. Will be the symbol
* `notRead` before the config is initially read, and after that it can
* potentially be `null` for an empty yaml file.
*
* This is the _raw_ config before any overrides are applied.
*
* As we have a notion of a _current_ config we rely on a BehaviorSubject so
* every new subscription will immediately receive the current config.
*/
private readonly rawConfigFromFile$ = new BehaviorSubject(notRead);
private readonly config$: Observable<RawConfig>;
constructor(readonly configFile: string) {
this.config$ = from(this.rawConfigFromFile$).pipe(
filter(rawConfig => rawConfig !== notRead),
map(rawConfig => {
// If the raw config is null, e.g. if empty config file, we default to
// an empty config
if (rawConfig == null) {
return new ObjectToRawConfigAdapter({});
}
if (isPlainObject(rawConfig)) {
// TODO Make config consistent, e.g. handle dots in keys
return new ObjectToRawConfigAdapter(rawConfig);
}
throw new Error(`the raw config must be an object, got [${typeDetect(rawConfig)}]`);
}),
// We only want to update the config if there are changes to it
skipWhile(isEqual)
);
}
/**
* Read the initial Kibana config.
*/
public loadConfig() {
const config = getConfigFromFile(this.configFile);
this.rawConfigFromFile$.next(config); // Argument of type 'any' is not assignable to parameter of type 'unique symbol'.
}
public stop() {
this.rawConfigFromFile$.complete();
}
/**
* Re-read the Kibana config.
*/
public reloadConfig() {
this.loadConfig();
}
public getConfig$() {
return this.config$;
}
}There was a problem hiding this comment.
Can you just do something like this instead?
const notRead = Symbol('config not yet read');
.....
private readonly rawConfigFromFile$ = new BehaviorSubject<any>(notRead);or
const notRead = Symbol('config not yet read');
.....
private readonly rawConfigFromFile$ = new BehaviorSubject<symbol | Record<string, any>>(
notRead
);
.....
map((rawConfig: Record<string, any>) => {There was a problem hiding this comment.
The former works! I wonder why compiler doesn't complain about unique symbol not being any type when done this way.
💔 Build Failed |
|
retest |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
|
@azasypkin @spalger ready for review, CI green |
|
ACK: can review it tomorrow morning (CEST) if Spencer doesn't review it earlier today. |
spalger
left a comment
There was a problem hiding this comment.
Things look good, pulled the PR and things seem to work well, and CI is green, so LGTM.
I would suggest removing the from() calls except when they're really necessary to convert a non-observable value to an observable.
|
|
||
| const configs = configService.atPath('key', ExampleClassWithStringSchema); | ||
| const exampleConfig = await k$(configs)(first(), toPromise()); | ||
| const exampleConfig = await from(configs) |
There was a problem hiding this comment.
I think it's reasonable to expect that the observables from configService.atPath() are observables that have .pipe() and .toPromise() methods, so I don't think we need to use from() in all the places we're using it now.
| @@ -34,7 +35,9 @@ test('returns config at path as observable', async () => { | |||
| const configService = new ConfigService(config$, defaultEnv, logger); | |||
|
|
|||
| const configs = configService.atPath('key', ExampleClassWithStringSchema); | |||
There was a problem hiding this comment.
How would you feel about tweaking the name of configs to config$ since it's a config stream? I realize TypeScript takes care of that information for us, so it's not super important, but I would still find it easier to read PRs on Github if the observable variables used the $ postfix.
azasypkin
left a comment
There was a problem hiding this comment.
LGTM, don't have anything to add to the feedback Spencer has already left.
💔 Build Failed |
|
Dashboard test failed |
|
retest |
💔 Build Failed |
💚 Build Succeeded |
|
6.x/6.5: e07b7e8 |
Closes #17034