feat: implement DataSource.stop()

[bajtos]

Implement stop as an alias for disconnect. This way LB4 applications don't have to include custom stop implementation in every datasource file.

Intended datasource file:

@lifeCycleObserver('datasource')
export class DbDataSource extends juggler.DataSource
  implements LifeCycleObserver {
  static dataSourceName = 'db';

  constructor(
    @inject('datasources.config.db', {optional: true})
    dsConfig: object = config,
  ) {
    super(dsConfig);
  }
}

Compare with the current template:

@lifeCycleObserver('datasource')
export class DbDataSource extends juggler.DataSource
  implements LifeCycleObserver {
  static dataSourceName = 'db';

  constructor(
    @inject('datasources.config.db', {optional: true})
    dsConfig: object = config,
  ) {
    super(dsConfig);
  }

  /**
   * Start the datasource when application is started
   */
  start(): ValueOrPromise<void> {
    // Add your logic here to be invoked when the application is started
  }

  /**
   * Disconnect the datasource when application is stopped. This allows the
   * application to be shut down gracefully.
   */
  stop(): ValueOrPromise<void> {
    return super.disconnect();
  }
}

Please note this change still allows app developers to add custom logic to start/stop step:

@lifeCycleObserver('datasource')
export class DbDataSource extends juggler.DataSource
  implements LifeCycleObserver {
  static dataSourceName = 'db';

  constructor(
    @inject('datasources.config.db', {optional: true})
    dsConfig: object = config,
  ) {
    super(dsConfig);
  }

  stop(): ValueOrPromise<void> {
    // add my custom shutdown steps
    return super.stop();
  }
}

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Commit messages are following our guidelines

[bajtos]

@slnode test please

[jannyHou]

Adding a stop function as alias looks reasonable to me.
I am not quite sure about how a lb4 datasource is connected, in the booter? Or some code in @loopback/repository establishes the connection? I am wondering, when apply the observer on a datasource, would the start/stop function interfere with the existing connect/disconnect mechanism?

[emonddr]

Approved, but please fix that typo before landing PR.

[bajtos]

I am not quite sure about how a lb4 datasource is connected, in the booter? Or some code in @loopback/repository establishes the connection? I am wondering, when apply the observer on a datasource, would the start/stop function interfere with the existing connect/disconnect mechanism?

The constructor of DataSource and/or the connector is dealing with establishing the connection, it also depends on whether the connector provides connect implementation. There is also lazyConnect flag that's set from the environment variable LAZY_CONNECT by LB3 booter.

To my best knowledge, LB4 is delegating the logic of establishing the connection fully to loopback-datasource-juggler and the connectors.

My pull request preserves that behavior by NOT introducing start method as an alias for connect. It may be possible to implement that in the future, but that's out of scope of this pull request.

[bajtos]

@slnode test please

[bajtos]

@slnode test please

[jannyHou]

My pull request preserves that behavior by NOT introducing start method as an alias for connect. It may be possible to implement that in the future, but that's out of scope of this pull request.

thank you for the explanation! LGTM 🚢

[danysz]

Since this "stop" simple projects doesn't compile anymore

See here a brand new project made with latest updates
repository

[bajtos]

Thank you @danysz for reporting the issue. We are aware of the problem, see the discussion in loopbackio/loopback-next#5261

[bajtos]

loopbackio/loopback-next#5261 (comment)

Ouch, looks like strongloop/loopback-datasource-juggler#1835 was not as backwards compatible as I thought 🙈 While I could change juggler's typings to describe the return value of stop method as PromiseOrValue<void>, I feel such description would be incorrect and would make the stop method more difficult to use outside of LB4 life-cycle observer use-cases.

I was thinking about this a bit more and realized that we have to fix the typings because I don't see any other way how to avoid breaking existing LB4 applications.

I opened a new PR in juggler, I would like to get it approved, merged and published today. See
#1838

[bajtos]

I published loopback-datasource-juggler@4.21.1 that should fix the issue. I apologize for the inconvenience caused 🙈