KevinAst / feature-u

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

[DOC] Difference between withFassets signature and example in UI Composition #29

Closed Okazari closed 4 years ago

Okazari commented 4 years ago

Hey !

In the documentation i believe that the signature given for withFassets is out of sync with an example in UI Composition.

image

Reading this i expect it to work like this :

withFassets(mapFassetsToProps)(Component)

but in the UI Composition the example is the following.

image

I'm pretty sure the example si out of date. Am i right ?

KevinAst commented 4 years ago

The docs are in fact up-to-date and accurate.

There are actually two ways to invoke withFassets() (please read the "two ways" note in the API more closely):

  1. You can invoke it the way you have suggested, which is the more classic approach. However this is only needed if you wish to perform "functional composition" on the returned HoF.

  2. You can invoke it the way that is referenced in UI Composition. This merely is a short-cut to directly pass back the HoC (NOT the HoF).

Note: Both techniques are shown in two separate examples within the API.

Also Note: The withFassets() function uses named parameters (which I much prefer for this particular API), making the parameter order insignificant. This is only real difference to the "classic" HoC API.

</Kevin>

Okazari commented 4 years ago

Thanks for your response.

With the signature i expected this call to be working :

withFassets(mapFassetsToProps, Component)

With your addition in the two ways section, i also expect it to works more classicaly like this :

withFassets(mapFassetsToProps)(Component)

I was just surprised by this third way of calling it :

withFassets({
  mapFassetsToProps: {},
  component: Component,
})

I think it could be an addition to precise that withFassets can be called with named parameters (just in case someone like me wonder where this signature comes from ;) !)

Thanks again for your time.

To be more precise for our usecase :

We're using it in a compose() function that compose HOF, that's why i was wondering about the third signature (that return the HOC and not the HOF)

KevinAst commented 4 years ago

I am unaware that the function would work with positional parameters. To my knowledge, from looking at the implementation, it should only operate using named parameters.

I would expect calling it like this:

withFassets(mapFassetsToProps, Component)

would generate the following error:

withFassets() parameter violation: unrecognized positional parameters (only named parameters can be specified)

Are you quite certain this works? If so, it befuddles me.

</Kevin>

Okazari commented 4 years ago

Hmm no i didn't try this syntax and it may not work then.

I just kind of exepected this to work reading the signature :

withFassets(mapFassetsToProps, [component]) => HOC | HOF

Reading this i consider that :

I may not be reading this well :thinking:

Okazari commented 4 years ago

To add an exemple : That's how it's mean to be read in lodash documentation

https://lodash.com/docs/4.17.15#join

_.join(array, [separator=','])

Meaning array is mandatory as 1st parameter, and separator is optionnal as 2nd (and default to ,)

KevinAst commented 4 years ago

Yes the function only accepts named parameters, and the component parameter is optional (it's existence determines whether the HoF or HoC is returned). Keep in mind that because they are named parameters, the parameter order does NOT matter.

Okazari commented 4 years ago

I completly understand how it does works, actually i'm just arguing that the signature doesn't reflect the fact that it should be used with named parameters :wink:

IMHO i would write the signature this way to reflect the named parameters :

withFassets({ mapFassetsToProps, [component] }) => HOC | HOF

What do you think ?

KevinAst commented 4 years ago

I think you are suggesting the withFassets() signature in the API docs should reflect the usage of named parameters.

The problem is that I am using JSDoc to auto generate the API, which does not support named parameters syntax (to my knowledge).

I compromised by placing the following note in the description of the API:

All other placed where I have control of the signature, I do show it with named parameters ... for example: Core API

Okazari commented 4 years ago

Understood :) ! That's better than nothing. Thanks for your time ;) !

I would have make a PR accordingly, but i didn't figure where to find this piece of doc. And now i understand why since it's generated.

Core API doc looks perfect :)