whichsteveyp / react-access Goto Github PK

Currently, <RequireForAccess/> uses the div soup strategy for rendering children. The initial reason for this was to support use cases like this:

<div className="icon-menu">
  <UserIcon/>
  <UserIcon/>
  <RequireForAccess permissions={['admin]}>
     <AdminIcon/>
     <AdminIcon2/>
  </RequireForAccess>
  <UserIcon/>
</div>

However, if .icon-menu is expecting to be display: flex then this will break the layout as technically the two admin icons are now in a single <div className="react-access-valid"/> container. You'd have to style that as well which leads folks to using CSS selectors specifically for our lib (potentially).

The alternative, of course, is users could just wrap each icon in a <RequireForAccess/> wrapper, regardless if they are intended to be shared sections. If that is the solution, should we move to enforcing that only one child be passed to this component and remove the extra <div/>?

@kmdeepthi and I spoke offline and she didn't seem to mind leveraging multiple wrappers with the single child workflow, and I think @jmeas had said mostly the same.

We're planning on adding #14 behavior, which could be used to solve my use case as well that lead to the initial API design. The other thing is, React 16 could very likely introduce fiber just around the corner which would allow for multiple children to be passed in anyway. Given that, I think we should move to a <div/>-less solution which would likely be better long term any how.

Now that we have tests, we should run them. It would be ideal to use this as a baseline for what versions of react we can support as a peerDependency.

• Integrate Travis for PRs/Pushes
• Configure travis suite to run multiple react versions
• Add code coverage reporting
• Add missing tests for context wrapper

We use enumerated permissions - therefore we would like to pass in numbers as permissions. As far as I can tell, the only thing preventing this from working is the proptypes assertion. Is there a greater reason for this?

Right now we depend on lodash but we're only using 1 function for it, intersection. It might be worthwhile to remove the dependency altogether and find a simpler default solution while encouraging users to provide their own validator implementations that would be more robust & customizable for their data formats.

The current API I used in one of my apps should be "good enough to get started", if we can extract that and publish a dist/ version that would get the first alpha out for consideration. Additional issues & prod release related issues can be filed after that.

Once the new Context API lands, we'll need to rethink this library and perhaps its overall usage / value. I put together some thoughts on what that might look like in this gist.

I'd love some feedback on this general direction and library, but I think the next major release could include these changes and perhaps a few others as well.

Recently saw @vinodkl use this.context.authorizeAccess() && <Foo/> which makes total sense. Would love to expose a connector that hoists from context to props so that consumers can leverage just the function call as well if they dislike the JSX (or don't need it for a special case).

It would be great to lint the src/ & test/ directories for better contribute-ability. Well and the code would likely benefit from it as well.

• Add lint task runners & appropriate .eslintrc files for src/ and test/
• Update package scripts for linting, possibly include it in a build step or pre-commit hook as well?

Per #16 (comment) we might want to use a MAIN_RUN variable or something for enhancing this behavior.

This could also include validating that this module works on different versions of node to ensure SSR capability.