KevinAst / feature-u

Feature Based Project Organization for React
https://feature-u.js.org/
MIT License
91 stars 6 forks source link

Modify validateFeatureContent to accept a list of all of the activeFeatures #15

Closed liamzdenek closed 6 years ago

liamzdenek commented 6 years ago

Currently, validateFeatureContent is invoked once per feature. This has some downsides:

My proposal is to pass activeFeatures to validateFeatureContent. That means this logic could mostly be relocated unmodified to a more proper lifecycle hook. I'm not really strong enough on the semantics of these hooks to know whether this is possible or preferable.

If it's possible to perform validations like this in other hooks, why does validateFeatureContent even exist? I'm inclined to always return valid and then check during the assemble stage.

[1] https://github.com/KevinAst/feature-redux/blob/master/src/reducerAspect.js#L373

KevinAst commented 6 years ago

Hello @liamzdenek,

First of all, I'm curious ... Are you actually developing an Aspect plugin using the Extension API? The only reason I ask, is that normal day-to-day usage of feature-u does not involve Extending feature-u (which is a more advanced topic).

On to your question ...

The Aspect object contains a series of Life Cycle Hooks that are invoked under the control of feature-u (launchApp()).

In general, an Aspect's responsibility is to:

By default, an Aspect automatically extends the Feature object by allowing it's AspectContent to be "cataloged" in the Feature object using the Aspect.name as it's key. As an example, a fooAspect (Aspect.name: 'foo') will permit a Feature.foo: fooContent construct.

The life cycle hook that you reference (validateFeatureContent()) has a very focused responsibility ... that is to validate the Feature.foo: fooContent when it is supplied. This content is specific to the fooAspect, so it is appropriate that it be given this responsibility.

The code snippet you reference is in a different life cycle hook (assembleFeatureContent()), which is handling the situation where no feature has supplied any fooContent. This is something that is quite different that validating fooContent when it is supplied.

Also in regard to your blacklist reference (I'm not sure if this is what you are referring to) but please note that Feature properties are already tightly controlled ... you can only specify built-in aspects, or those that are part of an Aspect extension.

Hopefully this helps.

KevinAst commented 6 years ago

Working as designed ... see explanation (above).