Skip to content

Kill kbn_observable and @kbn/observable#21944

Merged
spalger merged 10 commits intoelastic:masterfrom
rhoboat:rm-kbn-observable
Aug 16, 2018
Merged

Kill kbn_observable and @kbn/observable#21944
spalger merged 10 commits intoelastic:masterfrom
rhoboat:rm-kbn-observable

Conversation

@rhoboat
Copy link
Copy Markdown

@rhoboat rhoboat commented Aug 13, 2018

Closes #17034

@rhoboat rhoboat requested a review from azasypkin August 13, 2018 20:55
@rhoboat rhoboat added chore WIP Work in progress Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform labels Aug 13, 2018
@rhoboat
Copy link
Copy Markdown
Author

rhoboat commented Aug 13, 2018

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');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why do need this change? The type should be inferred.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The very first value will be symbol, but consequent values will be config's.

Copy link
Copy Markdown
Author

@rhoboat rhoboat Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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$;
  }
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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$;
  }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>) => {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former works! I wonder why compiler doesn't complain about unique symbol not being any type when done this way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epixa Here's the weirdness I was talking about.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@rhoboat
Copy link
Copy Markdown
Author

rhoboat commented Aug 14, 2018

retest

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@rhoboat rhoboat requested a review from spalger August 14, 2018 20:55
@rhoboat rhoboat added review and removed WIP Work in progress labels Aug 14, 2018
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@rhoboat
Copy link
Copy Markdown
Author

rhoboat commented Aug 15, 2018

@azasypkin @spalger ready for review, CI green

@azasypkin
Copy link
Copy Markdown
Contributor

ACK: can review it tomorrow morning (CEST) if Spencer doesn't review it earlier today.

@azasypkin azasypkin self-requested a review August 15, 2018 14:52
Copy link
Copy Markdown
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, don't have anything to add to the feedback Spencer has already left.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@rhoboat
Copy link
Copy Markdown
Author

rhoboat commented Aug 16, 2018

Dashboard test failed

@rhoboat
Copy link
Copy Markdown
Author

rhoboat commented Aug 16, 2018

retest

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@spalger spalger merged commit b6fdd49 into elastic:master Aug 16, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 17, 2018
spalger pushed a commit that referenced this pull request Aug 17, 2018
@spalger
Copy link
Copy Markdown
Contributor

spalger commented Aug 17, 2018

6.x/6.5: e07b7e8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Feature:New Platform review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v6.5.0 v7.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants