embarklabs/embark

RFC: Embark Engine constructor arguments type

0x-r4bbit opened this issue · 0 comments

Embark's Engine constructor parameters seem to have grown historically and cause some inflexibility in certain scenarios. The goal of this RFC is to revisit what's really needed to create an Engine instance, so it can stay environment agnostic and is easily consumable by other programs, such as the Embark CLI, and then come up with a spec'ed type definition that encodes the API everyone agreed upon.

Existing constructor API

Let's take a look at the existing constructor API to identify what needs potentially improvement:

https://github.com/embark-framework/embark/blob/1161cfa617d72f3b7de96dca59137262e04279f5/packages/core/core/src/engine.ts#L61-L78

There are a couple of things that stand out:

  1. options.client specifies the blockchain client used in commands like run and console. Given that it's theoretically possible to not use any blockchain at all, or even multiple ones, we probably don't want to tie a single client name on an Engine instance.
  2. options.embarkConfig is actually either some config object, or embark.json as default. We should probably introduce two separate configuration options for that and at the same time, consider if the lifetime of such an embarkConfig is potentially shorter than an Engine instance. In which case we probably want to retrieve it later and on-demand, similar to options.client.
  3. options.version is configurable, which should not be the case. We probably don't want consumers to create instances of Embark's core and decide what version it is.
  4. options.context doesn't carry any semantic value which makes it hard to even understand what it's for and why it exist. Maybe we can discuss what's the use case for it and introduce a more specific API?
  5. options.useDashboard is a flag that is really only relevant when in CLI environments that Engine shouldn't know anything about.
  6. options.webserverConfig, similar to other options, is something that may not actually be used. It's the configuration for a webserver that Embark spins up when the basic-pipeline is used, which we've deprecated in v4.
  7. options.webpackConfigName, just like webserverConfig is only needed if someone actually registers the basic-pipieline with Engine.
  8. options.package was actually a bandaid solution introduced in #2016 because some solc plugin relies on config.package.dependencies somewhere deep down in Embark. We should not expose this as an API requirement (which it is right now as Embark otherwise breaks).

In addition, it's worth noting that most of these configuration values don't make a lot of sense in other environments than Embark CLI, so it's important to iron this in order to make Engine work in environments like browsers.

Last but not least, it's also very important that, while those are all options, some of them are actually required to make Embark work and can't just be omitted (e.g. options.events and options.ipcRole.

It's probably better to encode those as required in the API and have other configuration values indeed optional.

In fact, it turns out that there are some properties of Engine that aren't actually initialized in its constructor, such as config and plugins, which is a problem because these essential properties are potentially undefined at run-time. We need to ensure that these things exist when an instance of Engine is created.

API Proposal

class Engine {

  version: string;

  constructor(
    private eventbus: EmbarkEmitter as Events,
    private config: Config,
    private options?: EngineOptions = {}
  ) {
    // ...
  } 
}

interface EngineOptions {
  env?: BuiltInEnv | string; // defaults to `BuiltInEnv.Development`
  locale?: SupportedLanguage; // provided by `embark-i18n`, defaults to `SupportedLanguage.En`
  logFile?: string;
  ipcRole?: IPCRole; // defaults to `IPCRole.Server`
  singleUseAuthToken?: boolean; // defaults to `false`
}

enum BuiltInEnv {
  Development = 'dev',
  Production = 'prod'
}

enum IPCRole {
  Server = 'server',
  Client = 'client'
}
  • Events and Config are now required parameters. Still need to figure out what we can best do with Plugins. At the moment it's created by Config.loadConfigFIles() which feels a little backwards. So that needs some fine-tuning as well.
  • options.client should be passed down to the dedicated blockchain module in the APIs/commands where it's actually needed
  • Same goes for options.webserverConfig and options.webpackConfigName
  • I think embarkConfig is also something that should be loaded on-demand and not when an instance of Engine is created, but happy to discuss this.
  • Engine.version should be determined internally and not passed as a constructor argument

Still need to learn more about what options.context is about. Maybe @iurimatias can chime in here?

Usuage

Obviously, any consumer (such as Embark CLI) will be able to import all necessary types and configure/instantiate Engine respectively:

import { Config, Engine, EngineOptions, Events } from 'embark-core';

const eventbus = new Events(...);
const config = new Config(...);

const options: EngineOptions = {
  ...
};

new Engine(eventbus, config, options);

Since Events and Config (and potentially Plugins [TBD])` need configuration and parameters themselves, and we don't necessarily want to require consumers to deal with that, we can introduce a factory function that makes creating engines easy:

import { createEngine, EngineOptions } from 'embark-core';

const options: EngineOptions = {
  // ...
};

const engine = createEngine(options);

// `options` is still optional so this works the same:
const engine = createEngine();