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

@skele/system package #104

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

@skele/system package #104

wants to merge 54 commits into from

Conversation

ognen
Copy link
Member

@ognen ognen commented Mar 8, 2018

The package contains the micro-framework used to build skele apps.

@ognen ognen added discussion A discussion issue or PR WIP Work in progress: a PR is labeled WIP is not to be merged labels Mar 8, 2018
@ognen ognen requested review from andon and bevkoski March 8, 2018 15:46
packages/system/README.md Outdated Show resolved Hide resolved
Copy link
Member

@andon andon left a comment

Choose a reason for hiding this comment

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

Mostly grammar nazi.
Lifecycle for stoping is also unclear to me.

packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/README.md Outdated Show resolved Hide resolved
packages/system/src/__tests__/subsystem.js Outdated Show resolved Hide resolved
packages/system/src/__tests__/system.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@bevkoski bevkoski left a comment

Choose a reason for hiding this comment

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

Thanks for the extensive documentation! I found going through it really interesting. The extensions part was not very clear to me at first, but the nice test coverage helped me understand it better.

Some grammar police from me as well and some organizational remarks.


```javascript
const hello = Unit({
/* ... from previous examlle */
Copy link
Collaborator

Choose a reason for hiding this comment

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

examlle -> example

})()
```

I.e. `System` is a just convinience method that gives you an _instantiated_ subsystem.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, the difference between systems and subsystems from a user's point of view is very minor. System instantiates a subsystem, while Subsystem is used to instantiate a subsystem.

Since a system can be built of both units/subsystems and their instances, it might be a good idea to expose only one of them (most probably System). For the sake of simplicity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, "a just" -> "just a".


## Lifecycle

A Unit can optionally can define a `start()` and a`stop()` method. The encompasing
Copy link
Collaborator

Choose a reason for hiding this comment

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

can optionally can -> can optionally

* The extensions defined in the surounding subsystem will propageate into the encapsulated one.
In other words units in the outer subsystem can contribute to units in the encapsulated one.
* The extensions defined in the encapsulated subsystem _will not_ propagate to the outer one
In other words, including a subsystem doesn't into another doesn't risk unwanted
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't into another doesn't risk -> doesn't risk

By making `productRoutes` a dependency of `tenantRoutes` we make sure the in the order in which
extensions are passed to the `router`, the tenant routes will always come after product routes.

To make this intent more clear, we've made `after` an alias for `using`. So you would write:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! It really contributes to clarity.

expect(system.unit2.prop2.prop1).toEqual(true)
})

test('throws in case of an unsatisfied dependency (we could relax this, I guess?)', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine as it is. Providing the declared dependencies should not be optional. A unit will most likely not be able to function without its dependencies.

Maybe, in the future, we can introduce the concept of optional dependencies. For example, if a logging dependency is provided, the unit will do logging, otherwise it will not.

packages/system/src/__tests__/system.js Outdated Show resolved Hide resolved
packages/system/src/__tests__/system.js Outdated Show resolved Hide resolved

export const pipe = (...fs) => x => fs.reduce((r, f) => f(r), x)

export const flow = (value, ...fns) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this the same as this? Maybe it's a good idea to have a util or data package, which other packages will depend on. It could contain shared functionalities such as this one.

"react",
"react-native",
"redux",
"redux-saga",
Copy link
Member

Choose a reason for hiding this comment

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

No need for redux-saga, we don't use it any more.

An **extension slot** is a formal declaration of a behavior, related to a _Unit_ that
can be extended (by means of addition or override/replacement) by **dependent Units**.

Lastly, an **extension** is the addition / override that a a **dependent Unit**
Copy link
Member

Choose a reason for hiding this comment

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

remove one a in front of dependent Unit

function.

```javascript
import { System, Unit } from '@skele/system'
Copy link
Member

Choose a reason for hiding this comment

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

Do you need System here?

```

A Unit can also be defined using a funciton, in which case
the framework expects that the funciton returns the Unit itself.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here it is the right place to mention why we have this additional way of Unit creation (dependent Units).

package.json Outdated
"es5": "yarn run es5:classic && yarn run es5:config && yarn run es5:components",
"es5:classic": "babel -q packages/classic/src --out-dir packages/classic/dist/es5",
"es5:config": "babel -q packages/config/src --out-dir packages/config/dist/es5",
"es5:components": "babel -q packages/components/src --out-dir packages/components/dist/es5",
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to create clean/build tasks for system package?

@ognen ognen force-pushed the features/system-package branch from 537e8bf to c25e974 Compare December 26, 2018 19:14
ognen added 28 commits June 5, 2019 16:48
Everything is an ext....
... Also introduced currying. Need to figure out a consitent applicaiton (e.g. modifiers are curried, terminals not).
You can pass the test as a funciton which would be evaluated only in DEV env. Use it to express
expensive tests.
done: query and querySpecs tests
missing: dependency tests
Ext <=> extension definition
Extension <=> the instance obtained from the Ext
props.ext => props.extFactory
I need toHaveBeenCalledBefore expectation.
Queries can now take an order param:
[slot, pred, order]
or
{
  '@@skele/extOf': slot,
 '@@skele/order': order
}

Order is either
 - order.definition (i.e. in the order they've been defined
 - order.topological (i.e. dependencies come earlier than dependant)

querySpecs -> queryExts
... browser compatibility.
system package done.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion A discussion issue or PR WIP Work in progress: a PR is labeled WIP is not to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants