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

Simple, robust and performant API #597

Closed
wants to merge 28 commits into from

Conversation

Projects
None yet
5 participants
@rofrischmann
Copy link
Owner

commented Aug 15, 2018

ToDo

  • Update API docs
  • Update "Usage with React/Inferno/Preact" docs
  • Migration guides
  • Simplify FelaComponent
  • use jest-react-fela for tests
  • allow arrays as style prop (applying combineRules automatically)
  • open combineRules to accept objects
  • sync release versions
  • Implement bullet-proof extendAPI for FelaComponent
  • CodeMods for FelaComponent changes
  • CodeMods fro FelaTheme changes
  • CodeMods for createComponent => FelaComponent
  • CodeMods for connect => FelaComponent

Description

This PR probably introduces the biggest shift since first releasing the React bindings for Fela.
We are deprecating createComponent, createComponentWithProxy, connect and withTheme all at once.

The new way to go is FelaComponent and FelaTheme.
Yet, don't worry! Before merging this PR we will provide useful guides, code mods and replacement helpers to ship a smooth migration path.

Questions

  • Can we support fallback for FelaComponent's API changes?
  • Can we reimplement connect / createComponent with FelaComponent only?
  • Does anyone want to maintain connect / createComponent?

Versioning

Major

  • fela-bindings
  • react-fela
  • preact-fela
  • inferno-fela

Minor

  • fela

Patch

Checklist

Quality Assurance

You can also run yarn run check to run all 4 commands at once.

  • The code was formatted using Prettier (yarn run format)
  • The code has no linting errors (yarn run lint)
  • All tests are passing (yarn run test)
  • There are no flow-type errors (yarn run flow)

Changes

If one of the following checks doesn't make sense due to the type of PR, just check them.

  • Tests have been added/updated
  • Documentation has been added/updated
  • My changes have proper flow-types
@brianespinosa

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

@levithomason in case you did not see this

@rofrischmann

This comment has been minimized.

Copy link
Owner Author

commented Aug 16, 2018

@brianespinosa @levithomason Any issues/concerns which those changes? I'm very open to collaborate and help out =)

@brianespinosa

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

@rofrischmann I was mainly just letting him know about the incoming updates since we are both working on large projects that use Fela. 👍

@rofrischmann

This comment has been minimized.

Copy link
Owner Author

commented Aug 16, 2018

Ah I see :)
If there’s anything, don’t hesitate to catch me on Twitter or Gitter :p

May I ask which projects and could we share it in the “Users” list in the Readme by any chance?

@brianespinosa

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

Stardust UI is what @levithomason is working on, which will eventually be used as the foundation for a new version of Semantic UI React.

My projects are with Espressive, our application is called Barista. Enterprise software.

@levithomason

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

Stardust UI has long-term lofty goals to produce a specification for standardized UI components and component anatomy. With this specification and component anatomy canon, we are able to provide framework agnostic utilities for accessibility, styling, and state management. These utilities are shareable across teams and platforms and have the potential to normalize implementations and share features/fixes across frameworks.

Fela is currently the library being used in Stardust for styling. We love the modular approach and focus on performance. It offers us the best chance so far to hitting our goals.

You can see the infancy of these ideas in Stardust's React bindings at stardust-ui/react.

At Microsoft we are using Stardust to forge our future component standards and practices. Initially, Stardust is supporting Teams. All of the topics we are tackling are in support of applications that need enterprise level components that can meet the demands of all users globally.

We've had mostly success with fela, but have also not used most of the utilities scheduled to be deprecated. Because of our requirements, we are mostly using the renderer directly in our own binding. We'll likely be focused on the renderer moving forward as we have our own robust base component patterns that we need to support.

@levithomason

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2018

I should also mention that we are striving to reduce the number of components in the render tree. This was a major factor in choosing to use the renderer directly. We are also considering ways to pass the renderer and theme in a single context channel. In fact, we are leaning toward having a single context channel for Stardust altogether.

The motivations for a single context channel are also due to trying to reduce the number of components in the render tree.

@rofrischmann

This comment has been minimized.

Copy link
Owner Author

commented Sep 7, 2018

@brianespinosa Mind adding Espressive to the list of users in the README btw? Guess for Stardust it's too early to do that

className,
theme,
})
}

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

I assume there are quite some people already using the FelaComponent API. The breaking changes to its API, while good, will require quite an elaborate process from users. I large code bases, this can be an issue.

Are you planning a codemod?

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 20, 2018

Author Owner

Yeah definitely. This is not going to ship without either fallback support (including deprecation warnings) or codemods =)

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

When it's time, we have quite a large codebase that makes extensive use of both the HoC APIs and the render-prop api, so happy to help test codemods

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 20, 2018

Author Owner

That'd be awesome!
One thing I stumbled over I'm not 100% sure how to convert is the following:

const Button = createComponent(({ color }) => ({
  color,
  fontSize: '15px'
})

const ExtendedButton = createComponent(({ bgColor }) => ({
  backgroundColor: bgColor,
  fontSize: '18px',
  lineHeight: 1.2
}, Button)

// => color:red;background-color:blue;font-size:18px;line-height:1.2
<ExtendedButton color="red" bgColor="blue" />

How would you implement composition with FelaComponent? Of course you can explicitly accept e.g. extend or sth. but that seems like recuring boilerplate to me.
If we figure out user-friendly composition with render-props, I'm all in for replacing everything.

We can discuss that on Gitter if you want to!

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

react-adopt seems to be the most popular solution for render-prop composition.

Right now, we use the as prop in a similar way to the second argument of createComponent, but only in the string for html primitive element case. What if we also allowed components there and automatically compose?

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

And it is a bit harder to understand because none of the implementation detail is abstracted away. That's both good and bad, I guess.

Just thinking out loud here, but maybe if style is an array, its items can be automatically be passed to combineRules?

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 20, 2018

Collaborator

Just realized - the renderProps object in the example above doesn't have a style property, only className and theme

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 21, 2018

Author Owner
  1. Yeah, I think we could expose the array-syntax with combineRules by default.
  2. Yep the renderProps right now only has className and theme, we would have to expose that, but I got a new idea anyways:
// ExtendedButton.js
import Button from './Button'

const extendedButton = ({ bgColor }) => ({
  backgroundColor: bgColor,
  fontSize: '18px',
  lineHeight: 1.2
})

const ExtendedButton = props => <Button extend={extendedButton} {...props} />

// => color:red;background-color:blue;font-size:18px;line-height:1.2
<ExtendedButton color="red" bgColor="blue" />

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 21, 2018

Author Owner

Ok. This API is freaking me out. It basically solves every issue and yet has the flexibility of createComponent.

This comment has been minimized.

Copy link
@brianespinosa

brianespinosa Sep 21, 2018

Contributor

Yes I like that. And unless I am missing something, it seems like it also solves the issue of adding lots of additional components to the React render tree.

rofrischmann added some commits Sep 20, 2018

),
{}
)
}

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 22, 2018

Collaborator

Wouldn't this potentially clobber media queries?

Consider:

const style = props => ({
  '@media (min-width: 600px)': { color: 'red', }
})

const MyComponent = props => (<FelaComponent style={style}  {...props}/>)

cont extendedStyle = props => ({
  '@media (min-width: 600px)': { backgroundColor: '#000', }
})

const MyExtendedComponent = props (<MyComponent extend={extendedStyle} {...props}

If I'm not mistaken, because assignStyle uses Object.assign, it only provides shallow merging of the resulting styles, and the media query in extendedStyle will not extend the one in style, but clobber it. From a user perspective, I'd expect an extend attribute to act in the same way as fela-plugin-extend.

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 22, 2018

Author Owner

Oh sure, should have added a comment. This Object.assign was only used to test the approach. I'll be using assignStyle which allows deep merging of objects and even merges arrays correctly with the next version (which also makes fela-combine-arrays obsolete and reduces complexity again).

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 22, 2018

Collaborator

I think we should also consider making fela-plugin-extend part of Fela's core, as to provide a consistent API throughout. Since assignStyle will be used by FelaComponent by default, it won't even add anything to the bundle size

This comment has been minimized.

Copy link
@rofrischmann

rofrischmann Sep 22, 2018

Author Owner

but the extend prop works different than the extend plugin.
The extend plugin is more like a nice API to do conditional style extensions. Not sure about that addition.
As soon as you add the plugin, you can of course also do sth. like:

const extend = ({ variant }) => ({
  fontSize: 20,
  extend: [{
    condition: variant === 'primary',
    style: {
      color: 'red',
      backgroundColor: 'lightred'
    }
  },{
    condition: variant === 'secondary',
    style: {
      color: 'blue',
      backgroundColor: 'lightblue'
    }
  }]
})

const ExtendedButton = () => <Button extend={style} />

This comment has been minimized.

Copy link
@TxHawks

TxHawks Sep 22, 2018

Collaborator

But extend is not only for conditional extension, it is also for things like this:

const fromDesktop = (prop, value) => ({ '@media (min-width: 1024px)': { [prop]: value, }, })

const style = props => ({
  extend:[
    fromDesktop('color', 'red'),
    fromDesktop('overflow', 'hidden'),
  ],
})

rofrischmann added some commits Sep 25, 2018

@rofrischmann rofrischmann referenced this pull request Sep 25, 2018

Open

Improving Documentation #615

0 of 17 tasks complete
@sgrishchenko

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

I am frightened by the desire to refuse to use the connect function. I think the possibilities of this functionality are greatly underestimated.
First of all, I did not see among the css-in-js solutions of other implementations of the multi-rule approach. This is a great premiere of how the stylization of a complex component can be separated from its business logic.
Secondly, the connect function allows you to extend styles, which automatically includes all the styling of the component in its public interface.
FelaComponent is conceptually not ready to implement this functionality. I really love Fela Connect ❤️and ask not to delete this API. Our company is actively using it.
https://ct.spotware.com/

@rofrischmann

This comment has been minimized.

Copy link
Owner Author

commented Nov 7, 2018

@sgrishchenko Hmm, I especially hate the multi-rule approach as I again have to name that shit (rather than mapping styles to components directly). You do not gain any benefit from connect expect for possibly mixing 2 different concerns. If you're styling a complex component with a single connect, have you considered composing smaller components? Apart from that, you should never style components with business logic, that's 2 separate concerns and can lead to super complex components (ofc thats just my opinion, but that might be the reason why you prefer connect at all).

In general, I would love to get rid of some complexity and cC / connect seem to be the biggest issues here. How about having separate packages with dedicated ownership? (Github should have tools for that). If you're happy maintaining connect/createComponent, there's nothing keeping you from doing that (maybe we should still refactor/simplify both, but yeah).

Nevertheless, would you be willing to maintain those hocs (maybe as a separate package here, react-fela-hocs or sth. like that)

@sgrishchenko

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

@rofrischmann This is suitable for me and I will be happy to do it. I don't like createComponent and I consider it redundant. Everything createComponent can do, FelaComponent can do the same. I just need to connect. I am also not interested in the support of connect implementations for preact and inferno. Perhaps a suitable option would be to transfer connect to the fela-tools package (combineMultiRules helper is already in this package).

@rofrischmann rofrischmann referenced this pull request Nov 9, 2018

Merged

Simplified FelaComponent API #636

7 of 7 tasks complete
@rofrischmann

This comment has been minimized.

Copy link
Owner Author

commented Nov 9, 2018

Closing this in favor of #636 which allows a more incremental approach. All improvements here have already been ported to #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.