Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SFX-99: Add Core module with registration mechanism #5

Merged
merged 62 commits into from
Jul 5, 2019
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
3686851
Set up @sfx/core package
jmbtutor Jun 21, 2019
8cc2764
Add Core class
jmbtutor Jun 21, 2019
ce46f63
Make Core the default export of the package
jmbtutor Jun 21, 2019
4d9b262
Set up sinon-chai
jmbtutor Jun 21, 2019
ded77f8
Call plugins' register() function on registration
jmbtutor Jun 21, 2019
c59df38
Split registration order assertion into another test
jmbtutor Jun 21, 2019
aa52103
Add dummy checkDependencies function
jmbtutor Jun 24, 2019
623dc1c
Throw error when there are unmet dependencies
jmbtutor Jun 24, 2019
1c2df93
Return list of missing dependencies instead
jmbtutor Jun 24, 2019
21ab9ec
Add more missing dependencies
jmbtutor Jun 24, 2019
5835401
Test positive dependency cases
jmbtutor Jun 24, 2019
67ee2de
Add TODO for documentation
jmbtutor Jun 24, 2019
579cba5
Spell out equal in full
jmbtutor Jun 24, 2019
aa3d901
Add registerPlugins core util method
jmbtutor Jun 24, 2019
e467fee
Add initPlugins function to core utils
jmbtutor Jun 24, 2019
9a8bfe4
Add readyPlugins core util function
jmbtutor Jun 24, 2019
993613e
Refactor getMissingDependencies() to accept plugins.
DanielMoniz-GBI Jun 24, 2019
35ea4c5
Rename getMissingDependencies to calculateMissingDependencies
jmbtutor Jun 25, 2019
c53c772
Annotate the type instead of casting
jmbtutor Jun 25, 2019
232c5d0
Include the new plugins when calculating dependencies
jmbtutor Jun 25, 2019
6a12f4e
Remove unnecessary type annotations
jmbtutor Jun 25, 2019
bad8648
Add names to the plugins
jmbtutor Jun 25, 2019
1a6a942
Restore sinon's default sandbox after each test
jmbtutor Jun 25, 2019
953feaa
Throw when dependencies are missing
jmbtutor Jun 25, 2019
9ca60f6
Call lifecycle methods in Core.register
jmbtutor Jun 25, 2019
e1b1c3a
Document CoreUtils
jmbtutor Jun 25, 2019
cd5ccd4
Add test for circular dependencies
jmbtutor Jun 25, 2019
f30903d
Fix type of depends
jmbtutor Jun 27, 2019
af26a48
Normalize function type declarations
jmbtutor Jun 27, 2019
f21ac84
Export the plugin interfaces
jmbtutor Jun 28, 2019
f9913c8
Assert that the entrypoint exports the proper members
jmbtutor Jun 28, 2019
ba063d2
Do not allow plugins to modify the registry
jmbtutor Jun 28, 2019
4484a0a
Create interface for plugin registry
jmbtutor Jun 28, 2019
f9cf7a5
Allow the package to be used in a JavaScript project
jmbtutor Jun 28, 2019
af6cb48
Export Core as named instead of as default
jmbtutor Jun 28, 2019
fb61a30
Add bare @sfx/core readme
jmbtutor Jun 28, 2019
6a799f8
Document the Core module
jmbtutor Jun 28, 2019
f8242d7
Document the plugin interfaces
jmbtutor Jun 28, 2019
44d90db
Document the Core utils module
jmbtutor Jun 28, 2019
bd1b758
Add crude instantiation documentation to README
jmbtutor Jun 28, 2019
3eafb1a
Generate core bundle
jmbtutor Jun 28, 2019
b9e6a70
Add description to package.json
jmbtutor Jul 3, 2019
ff5a280
Use new testing utils
jmbtutor Jul 3, 2019
52f48b8
Add changelog entry for Core
jmbtutor Jul 3, 2019
1442c9a
Run tests in subdirectories
jmbtutor Jul 3, 2019
55e8ac1
Fix typo
jmbtutor Jul 3, 2019
b5fe860
Remove unnecessary initialization
jmbtutor Jul 3, 2019
4b7ee8d
Format module names as code
jmbtutor Jul 3, 2019
efad980
Rename core.plugins to core.registry
jmbtutor Jul 3, 2019
7bb2650
Remove unused imports
jmbtutor Jul 3, 2019
6bf7d79
Use correct interface
jmbtutor Jul 3, 2019
af4cdef
Change wording of optional hooks to be less confusing
jmbtutor Jul 3, 2019
58f2e9a
Remove duplicated line
jmbtutor Jul 3, 2019
30c2f23
Revert yarn.lock
jmbtutor Jul 3, 2019
6baea5e
Make explicit that plugins have been initialized in the ready phase
jmbtutor Jul 4, 2019
047d81b
Remove superfluous empty lines
jmbtutor Jul 4, 2019
2b04d08
Change test name for clarity
jmbtutor Jul 4, 2019
702b658
Use different plugin values
jmbtutor Jul 4, 2019
9555c7e
Generate .d.ts files
jmbtutor Jul 5, 2019
00cb049
Declare esnext entrypoint
jmbtutor Jul 5, 2019
398430b
Enable webpack tree shaking of the Core package
jmbtutor Jul 5, 2019
1b5ac59
Add helper type to assert type equality
jmbtutor Jul 5, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/@sfx/core/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# SF-X Core

This package contains the SF-X Core class and related plugin interfaces.

## Usage

To use Core, simply instantiate it:

```js
const core = new Core();
```

## Registering a plugin

To register one or more plugins with Core, instantiate the plugins, then
pass them to `Core.register()`:

```js
const core = new Core();
const pluginA = new PluginA();
const pluginB = new PluginB();

core.register([pluginA, pluginB]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just take an arbitrary number of plugins as arguments instead of a single array? This need not be done right away, but the interface would feel simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepting an array allows us to accept registration options that apply to the batch in the future. Looking ahead to a possible "provides" implementation, one such option may be to specify which plugin, out of multiple plugins that provide the same name, should be exposed under that name.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably still accept options if we do some magic in the register() method. The plugins have to be actual plugins that conform to a specific interface, and the final argument could be an optional plain old object.

It's possible that this is considered not-cool in JavaScript, but it would still be an easier interface for our clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that if we do that, we would have no way to differentiate between a plugin and the options, especially if the options are optional (which it should be). In addition, I think the common case will be to register a number of plugins at the start, so an array is reasonable. I think I'll keep the array for those reasons.

```

A plugin may take configuration options in its constructor. Refer to the
plugin's documentation for details.
1 change: 1 addition & 0 deletions packages/@sfx/core/karma.conf.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require('../../../karma.conf.js');
21 changes: 21 additions & 0 deletions packages/@sfx/core/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"name": "@sfx/core",
"version": "0.0.0",
"description": "SF-X Core",
"main": "dist/index.js",
"scripts": {
"build": "../../../scripts/build.sh",
"dev": "nodemon --config ../../../nodemon.json --exec npm run build",
"test": "nyc mocha",
"tdd": "nodemon --config ../../../nodemon.test.json --exec npm test",
"test:browser": "karma start karma.conf.js",
"tdd:browser": "karma start karma.conf.js --no-single-run"
},
"repository": {
"type": "git",
"url": "https://github.com/groupby/sfx-logic.git",
"directory": "packages/@sfx/core"
},
"author": "",
"license": "MIT"
}
59 changes: 59 additions & 0 deletions packages/@sfx/core/src/core.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { Plugin, PluginRegistry } from './plugin';
import {
calculateMissingDependencies,
initPlugins,
readyPlugins,
registerPlugins,
} from './utils/core';

/**
* The core of the SF-X plugin system. This entity is responsible for
* managing plugins and provides a mechanism for plugins to communicate
* with each other.
*/
export default class Core {
/**
* The plugin registry. This object is a dictionary containing plugin
* names as keys and their corresponding exposed values as values.
*
* The preferred way to access plugins in the registry is through
* another plugin. Accessing the plugins through Core outside of a
* plugin is discouraged.
*/
plugins: PluginRegistry = Object.create(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be called registry instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should, I think it makes it more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

registry is better than plugins, as they need not be actual plugins, but rather the values exposed by each plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

If plugins has been renamed to registry, should the interface be renamed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface name is already PluginRegistry. Since it will be used outside of Core, I think it can stay the way it is.


/**
* Register one or more plugins with Core.
*
* Plugins given here are registered as a batch: the next lifecycle
* event does not occur until all plugins in this batch have
* experienced the current lifecycle event.
*
* The registration lifecycle is as follows:
*
* 1. **Registration:** The registry is populated with each plugin's
* name and the value that it wants to expose. Plugins cannot
* assume that other plugins have been registered.
* 2. **Initialization:** Plugins perform setup tasks. Plugins are
* aware of the existence of other plugins, but should not make use
* of their functionality as they may not yet be initialized.
* 3. **Ready:** Plugins may make use of other plugins.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we explicitly mention that plugins are initialized in the ready phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plugins are initialized in the init phase, not the ready phase. The ready phase exists as a hook to say that the registration process is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather I meant to say that we should mention that all plugins should have been initialized and their functionality is now available for use within the ready phase.

*
* This function ensures that all plugin dependencies are available
* before proceeding to register the plugins. Circular dependencies
* are supported. If dependencies are missing, an error will be
* thrown.
*
* @param plugins An array of plugin instances to regsiter.
*/
register(plugins: Plugin[]) {
const missingDependencies = calculateMissingDependencies(plugins, this.plugins);
if (missingDependencies.length) {
throw new Error('Missing dependencies: ' + missingDependencies.join(', '));
}

registerPlugins(plugins, this.plugins);
initPlugins(plugins);
readyPlugins(plugins);
}
}
2 changes: 2 additions & 0 deletions packages/@sfx/core/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default as Core } from './core';
export { Plugin, PluginMetadata, PluginRegistry } from './plugin';
83 changes: 83 additions & 0 deletions packages/@sfx/core/src/plugin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/**
* A plugin for use with the SF-X Core. Plugins supply functionality to
* the otherwise function-less Core. Plugins may depend on other
* plugins.
*
* A plugin must conform to this interface to be registered with Core,
* but it may define other methods and properties for internal use.
*/
export interface Plugin {
/**
* Plugin metadata. This object provides Core with the information
* necessary to register the plugin.
*
* Plugin authors may consider using a getter to implement this
* property.
*/
metadata: PluginMetadata;

/**
* The callback for the registration phase of the lifecycle. The
* plugin will receive a reference to the plugin registry (the object
* through which other plugins can be accessed) here, and the function
* is expected to return a value that will be exposed through the same
* registry. It is recommended to store a reference to both values in
* the plugin for later retrieval.
*
* The plugin cannot assume that other plugins are registered in this
* function.
*
* @param plugins The plugin registry containing all other plugins.
* @returns The value to expose in the registry.
*/
register: (plugins: PluginRegistry) => any;

/**
* The callback for the initialization phase of the lifecycle. The
* plugin should do as much setup as it can in this function.
*
* In this function, the plugin can assume that other plugins have
* been registered and are accessible through the plugin registry
* object given in the `register` function, but it cannot assume that
* the other plugins have been initialized. In other words, the plugin
* will know which plugins are available, but it cannot use them.
*/
init?: () => void;

/**
* The callback for the ready phase of the lifecycle.
*
* In this function, the plugin can assume that other plugins have
* been initialized.
*/
ready?: () => void;
}

/**
* Plugin metadata. Core will use the properties specified in this
* interface during the registration process.
*/
export interface PluginMetadata {
/**
* The name of the plugin. This name will be used as the key in the
* registry, so this name should be unique. By convention, this name
* should be a valid JavaScript identifier and be written in
* snake_case.
*/
name: string;

/**
* The names of all the plugins that this plugin has a hard dependency
* on (i.e. these plugins must be present for this plugin to
* function).
*/
depends: string[];
}

/**
* The type of the plugin registry. Each key/value pair corresponds to
* the name of a plugin and its exposed value.
*/
export interface PluginRegistry {
[key: string]: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript object keys are all strings. Do we need to specify a type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

A little explanation would be nice. Why do we need the type? Do we not get to decide what we type and what we do not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the PluginRegistry makes it more explicit that a plugin registry is a dictionary, and allows us to change the type in the future if we so decide. It also provides the assertion that the keys can indeed by anything. Using the generic type object doesn't convey the intentions and is less flexible.

Having to specify string is required as a side effect of the syntax. This is what I meant by "unfortunately, yes".

}
79 changes: 79 additions & 0 deletions packages/@sfx/core/src/utils/core.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { Plugin, PluginRegistry } from '../plugin';

/**
* Calculates the missing dependencies of the given plugins. The given
* plugins and the plugins in the registry are eligible to satisfy
* dependencies.
*
* @param plugins The plugins whose dependencies should be checked.
* @param registry The plugin registry containing all registered plugins.
* @returns An array of names of missing plugins.
*/
export function calculateMissingDependencies(plugins: Plugin[], registry: object): string[] {
const available = [
...Object.keys(registry),
...plugins.map(({ metadata: { name }}) => name),
];
const availableSet = new Set(available);

const required = plugins.reduce((memo, plugin) => {
return [...memo, ...plugin.metadata.depends];
}, []);
const requiredSet = new Set(required);

const difference = new Set(Array.from(requiredSet).filter((p) => !availableSet.has(p)));

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove some of the empty lines above.

return Array.from(difference.values()).sort();
}

/**
* Calls the `register` function of each plugin. The values returned by
* each plugin are added to the given registry.
*
* @param plugins The plugins to register.
* @param registry The registry into which to add the plugins.
* @returns An object containing the keys and values of the new items
* added to the registry.
*/
export function registerPlugins(plugins: Plugin[], registry: PluginRegistry): PluginRegistry {
const newlyRegistered = Object.create(null);

plugins.forEach((plugin) => {
const exposedValue = plugin.register(Object.create(registry));
const { name } = plugin.metadata;

newlyRegistered[name] = exposedValue;
});

Object.assign(registry, newlyRegistered);

Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably just remove this newline.

return newlyRegistered;
}

/**
* Calls the `init` function of each plugin. If a plugin does not
* exppose an `init` function, it is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* exppose an `init` function, it is ignored.
* expose an `init` function, it is ignored.

*
* @param plugins The plugins to initialize.
*/
export function initPlugins(plugins: Plugin[]) {
plugins.forEach((plugin) => {
if (typeof plugin.init === 'function') {
plugin.init();
}
});
}

/**
* Calls the `ready` function of each plugin. If a plugin does not
* expose a `ready` function, it is ignored.
*
* @param plugins The plugins to ready.
*/
export function readyPlugins(plugins: Plugin[]) {
plugins.forEach((plugin) => {
if (typeof plugin.ready === 'function') {
plugin.ready();
}
});
}
1 change: 1 addition & 0 deletions packages/@sfx/core/test/setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import '../../../../test/setup';
Empty file.
84 changes: 84 additions & 0 deletions packages/@sfx/core/test/unit/common/core.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { expect, sinon, spy, stub } from '../../utils';
import Core from '../../../src/core';
import * as CoreUtils from '../../../src/utils/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to get src/ on the path so that modules can simply start there. The above can cause issues if/when refactoring the directory structure needs to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using TypeScript's baseUrl and paths compiler options, which enables path mapping, but it looks like tsc does not output resolved paths.

There are two packages that look like they may solve that issue (namely tscpaths and ts-transformer-inputs), but they don't seem to be widely used, and one of them explicitly says that it's not ready for production. The oft-recommended module-alias is not suitable for us since this is a library and not an application. Doing it through Webpack is not an option since we don't use Webpack to build the package, and we don't want a single bundle for the whole package.

As much as I would like path aliasing, I don't think we can have it under our current circumstances unless we decide that the risk of using either tscpaths or ts-transformer-inputs is worth it.


describe('Core', () => {
let core: Core;

beforeEach(() => {
core = new Core();
});

describe('constructor()', () => {
it('should create an empty null-prototype plugins registry object', () => {
expect(core.plugins).to.be.empty;
expect(Object.getPrototypeOf(core.plugins)).to.be.null;
});
});

describe('register()', () => {
let calculateMissingDependencies = stub(CoreUtils, 'calculateMissingDependencies').returns(['x']);
let registerPlugins = stub(CoreUtils, 'registerPlugins');
let initPlugins = stub(CoreUtils, 'initPlugins');
let readyPlugins = stub(CoreUtils, 'readyPlugins');

beforeEach(() => {
calculateMissingDependencies = stub(CoreUtils, 'calculateMissingDependencies');
registerPlugins = stub(CoreUtils, 'registerPlugins');
initPlugins = stub(CoreUtils, 'initPlugins');
readyPlugins = stub(CoreUtils, 'readyPlugins');
});

it('should throw if dependencies are not met', () => {
const plugins: any = [
{
metadata: {
name: 'a',
depends: ['x'],
},
},
{
metadata: {
name: 'b',
depends: ['a'],
},
},
];
calculateMissingDependencies.returns(['x']);
Object.assign(core.plugins, { m: 'mm' });

expect(() => core.register(plugins)).to.throw();
expect(calculateMissingDependencies).to.be.calledWith(plugins, sinon.match(core.plugins));
});

it('should call the lifecycle events on the plugins in order', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be pedantic about the language clarity:

Suggested change
it('should call the lifecycle events on the plugins in order', () => {
it('should call the lifecycle events in order on the plugins', () => {

const plugins: any = [
{
metadata: {
name: 'a',
depends: [],
},
},
{
metadata: {
name: 'b',
depends: [],
},
},
];
calculateMissingDependencies.returns([]);

core.register(plugins);

expect(registerPlugins).to.be.calledWith(plugins, sinon.match(core.plugins));
expect(registerPlugins).to.be.calledWith(plugins, sinon.match(core.plugins));
expect(initPlugins).to.be.calledWith(plugins);
expect(readyPlugins).to.be.calledWith(plugins);
sinon.assert.callOrder(
registerPlugins,
initPlugins,
readyPlugins
);
});
});
});
Loading