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

jmbtutor · 2019-07-03T15:05:35Z

Core is the backbone of the entire SF-X system. It manages plugins and allows them to communicate with each other. The core package defines interfaces for plugins.

A registration mechanism is currently implemented. Unregistration will be added separately.

Use the template from the dummy sayt-plugin package.

sinon-chai makes assertions using chai expect and sinon less awkward

Each plugin will expose a `register` function that takes in a plugins object that contains a reference to all plugins and returns a value to expose.

CoreUtils.getMissingDependencies() now takes a list of plugins and the plugin registry. It uses this to first determine the list of dependencies, and second to determine which dependencies are not satisfied. TODO: The newly supplied plugins can satisfy their own dependencies, and this is not taken into account. WIP: Comment out tests and update signature of getMissingDependencies() WIP: Uncomment & fix one getMissingDependencies() test WIP: Update plugin in getMissingDependencies() to use interface. Fix remaining commented getMissingDependencies() test.

"Calculate" does not imply that the actual plugin is returned.

TypeScript can infer these types.

This keeps tests isolated.

Use the arrow function form.

jmbtutor · 2019-07-03T15:15:48Z

packages/@sfx/core/src/core.ts

+   * another plugin. Accessing the plugins through Core outside of a
+   * plugin is discouraged.
+   */
+  plugins: PluginRegistry = Object.create(null);


Should this be called registry instead?

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

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

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

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

packages/@sfx/core/src/core.ts

packages/@sfx/core/test/unit/common/utils/core.test.ts

packages/@sfx/core/test/unit/common/core.test.ts

packages/@sfx/core/test/unit/common/index.test.ts

packages/@sfx/core/src/utils/core.ts

JoeyDeol

Good work, just some comments to look over.

JoeyDeol · 2019-07-03T17:26:21Z

packages/@sfx/core/src/core.ts

+   * 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.


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

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.

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.

JoeyDeol · 2019-07-03T17:38:08Z

packages/@sfx/core/src/core.ts

+   * another plugin. Accessing the plugins through Core outside of a
+   * plugin is discouraged.
+   */
+  plugins: PluginRegistry = Object.create(null);


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

packages/@sfx/core/src/utils/core.ts

yarn.lock

packages/@sfx/core/test/unit/common/core.test.ts

DanielMoniz-GBI · 2019-07-03T18:18:18Z

packages/@sfx/core/README.md

+const pluginA = new PluginA();
+const pluginB = new PluginB();
+
+core.register([pluginA, pluginB]);


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.

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.

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.

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.

DanielMoniz-GBI · 2019-07-03T18:21:49Z

packages/@sfx/core/src/core.ts

+   * another plugin. Accessing the plugins through Core outside of a
+   * plugin is discouraged.
+   */
+  plugins: PluginRegistry = Object.create(null);


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

DanielMoniz-GBI · 2019-07-03T19:07:35Z

packages/@sfx/core/src/plugin.ts

+ * the name of a plugin and its exposed value.
+ */
+export interface PluginRegistry {
+  [key: string]: any;


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

Unfortunately, yes.

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?

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".

packages/@sfx/core/src/utils/core.ts

DanielMoniz-GBI · 2019-07-03T19:12:21Z

packages/@sfx/core/src/utils/core.ts

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


We can remove some of the empty lines above.

packages/@sfx/core/test/unit/common/utils/core.test.ts

DanielMoniz-GBI · 2019-07-03T19:27:10Z

packages/@sfx/core/test/unit/common/utils/core.test.ts

+    it('should return a map of new plugin keys to exposed values', () => {
+      const valueA = { a: 'a' };
+      const valueB = { b: 'b' };
+      const valueC = { c: 'c' };


We probably should have considered using varying datatypes for the values above. It makes it look like plugins have to expose objects, which is not true, and I find that distracts from the test.

`registry` is more in line with the surrounding terminology. It also allows us to use `plugins` for actual plugins at a later time.

JoeyDeol

All my feedback has been addressed, once the rest of the feedback has been resolved this should be ready to go.

DanielMoniz-GBI · 2019-07-04T14:37:08Z

packages/@sfx/core/test/unit/common/utils/core.test.ts

+      ]
+
+      const missing = calculateMissingDependencies(plugins, registry);
+


This is fine, but it's not a convention we agreed upon. I was also not making the point for optimization, but rather to minimize noise and to generally make the file feel more readable and consistent with how we'd be coding elsewhere. I see no reason to blindly follow rules like Arrange Act Assert if they aren't really providing value. The tests would still be in the spirit of those guidelines.

DanielMoniz-GBI · 2019-07-04T14:45:19Z

packages/@sfx/core/README.md

+const pluginA = new PluginA();
+const pluginB = new PluginB();
+
+core.register([pluginA, pluginB]);


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.

DanielMoniz-GBI · 2019-07-04T14:46:25Z

packages/@sfx/core/src/plugin.ts

+ * the name of a plugin and its exposed value.
+ */
+export interface PluginRegistry {
+  [key: string]: any;


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?

DanielMoniz-GBI · 2019-07-04T15:03:23Z

packages/@sfx/core/test/unit/common/index.test.ts

+  it('should export the PluginRegistry interface', () => {
+    // The following should cause a compiler error if the two interfaces are not equal.
+    const plugin_registry_is_assignable_to_plugin_registry_export: PluginRegistryExport = {} as PluginRegistry;
+    const plugin_registry_export_is_assignable_to_plugin_registry: PluginRegistry = {} as PluginRegistryExport;


Yeah, I think this is a good use case for comments. This feels very noisy, and I found the variable names confused me more than anything.

All of that said, we're testing an index file here. The actual value in the tests feels limited to me, so I don't want to spend too much time on it.

If I add a helper function to assert that two interfaces are equal, I think it'll make the test much clearer.

Plugins need not return objects, though that is probably the most common type.

https://webpack.js.org/guides/tree-shaking/

The AssertTypesEqual asserts equality by ensuring that the two types are subtypes of each other.

jmbtutor and others added 30 commits July 3, 2019 10:47

Set up @sfx/core package

3686851

Use the template from the dummy sayt-plugin package.

Add Core class

8cc2764

Make Core the default export of the package

ce46f63

Set up sinon-chai

4d9b262

sinon-chai makes assertions using chai expect and sinon less awkward

Call plugins' register() function on registration

ded77f8

Each plugin will expose a `register` function that takes in a plugins object that contains a reference to all plugins and returns a value to expose.

Split registration order assertion into another test

c59df38

Add dummy checkDependencies function

aa52103

Throw error when there are unmet dependencies

623dc1c

Return list of missing dependencies instead

1c2df93

Add more missing dependencies

21ab9ec

Test positive dependency cases

5835401

Add TODO for documentation

67ee2de

Spell out equal in full

579cba5

Add registerPlugins core util method

aa3d901

Add initPlugins function to core utils

e467fee

Add readyPlugins core util function

9a8bfe4

Rename getMissingDependencies to calculateMissingDependencies

35ea4c5

"Calculate" does not imply that the actual plugin is returned.

Annotate the type instead of casting

c53c772

Include the new plugins when calculating dependencies

232c5d0

Remove unnecessary type annotations

6a12f4e

TypeScript can infer these types.

Add names to the plugins

bad8648

Restore sinon's default sandbox after each test

1a6a942

This keeps tests isolated.

Throw when dependencies are missing

953feaa

Call lifecycle methods in Core.register

9ca60f6

Document CoreUtils

e1b1c3a

Add test for circular dependencies

cd5ccd4

Fix type of depends

f30903d

Normalize function type declarations

af26a48

Use the arrow function form.

Export the plugin interfaces

f21ac84

jmbtutor commented Jul 3, 2019

View reviewed changes

geoffcoutts suggested changes Jul 3, 2019

View reviewed changes

jmbtutor added 3 commits July 3, 2019 14:52

Run tests in subdirectories

1442c9a

Fix typo

55e8ac1

Remove unnecessary initialization

b5fe860

JoeyDeol suggested changes Jul 3, 2019

View reviewed changes

DanielMoniz-GBI suggested changes Jul 3, 2019

View reviewed changes

jmbtutor added 7 commits July 3, 2019 15:31

Format module names as code

4b7ee8d

Rename core.plugins to core.registry

efad980

`registry` is more in line with the surrounding terminology. It also allows us to use `plugins` for actual plugins at a later time.

Remove unused imports

7bb2650

Use correct interface

6bf7d79

Change wording of optional hooks to be less confusing

af4cdef

Remove duplicated line

58f2e9a

Revert yarn.lock

30c2f23

JoeyDeol reviewed Jul 4, 2019

View reviewed changes

geoffcoutts approved these changes Jul 4, 2019

View reviewed changes

jmbtutor added 3 commits July 4, 2019 10:23

Make explicit that plugins have been initialized in the ready phase

6baea5e

Remove superfluous empty lines

047d81b

Change test name for clarity

2b04d08

DanielMoniz-GBI suggested changes Jul 4, 2019

View reviewed changes

jmbtutor added 5 commits July 4, 2019 11:50

Use different plugin values

702b658

Plugins need not return objects, though that is probably the most common type.

Generate .d.ts files

9555c7e

Declare esnext entrypoint

00cb049

Enable webpack tree shaking of the Core package

398430b

https://webpack.js.org/guides/tree-shaking/

Add helper type to assert type equality

1b5ac59

The AssertTypesEqual asserts equality by ensuring that the two types are subtypes of each other.

jmbtutor force-pushed the SFX-99 branch from 39012da to 1b5ac59 Compare July 5, 2019 16:05

DanielMoniz-GBI approved these changes Jul 5, 2019

View reviewed changes

JoeyDeol approved these changes Jul 5, 2019

View reviewed changes

jmbtutor merged commit 367de56 into master Jul 5, 2019

jmbtutor deleted the SFX-99 branch July 5, 2019 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmbtutor commented Jul 3, 2019

jmbtutor Jul 3, 2019

JoeyDeol Jul 3, 2019

DanielMoniz-GBI Jul 3, 2019

geoffcoutts Jul 3, 2019

jmbtutor Jul 4, 2019

JoeyDeol left a comment

JoeyDeol Jul 3, 2019

jmbtutor Jul 3, 2019

JoeyDeol Jul 4, 2019

JoeyDeol Jul 3, 2019

DanielMoniz-GBI Jul 3, 2019

jmbtutor Jul 4, 2019

DanielMoniz-GBI Jul 4, 2019

jmbtutor Jul 5, 2019

DanielMoniz-GBI Jul 3, 2019

DanielMoniz-GBI Jul 3, 2019

jmbtutor Jul 4, 2019

DanielMoniz-GBI Jul 4, 2019

jmbtutor Jul 5, 2019

DanielMoniz-GBI Jul 3, 2019

DanielMoniz-GBI Jul 3, 2019

JoeyDeol left a comment

DanielMoniz-GBI Jul 4, 2019

DanielMoniz-GBI Jul 4, 2019

DanielMoniz-GBI Jul 4, 2019

DanielMoniz-GBI Jul 4, 2019

jmbtutor Jul 4, 2019

		const requiredSet = new Set(required);

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

		]

		const missing = calculateMissingDependencies(plugins, registry);

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

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

Conversation

jmbtutor commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeyDeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeyDeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment