Marcisbee / radi

πŸŒ€Tiny (in size) front-end framework with no extra browser re-flows
https://radi.js.org
MIT License
948 stars 34 forks source link

Proposal for changes to API #17

Closed rafaelklaessen closed 6 years ago

rafaelklaessen commented 6 years ago

This is a proposal for changes to the user facing API of Radi. It's just a proposal, take it with a grain of salt.

Let me know what you think! πŸ˜„

Marcisbee commented 6 years ago

About everything else, we discussed this already and I would really like this API change.

rafaelklaessen commented 6 years ago

I don't know about getting rid of mixins since I don't entirely understand why they are necessary in the first place. I believe they could be avoided either way.

The reason I think extending Component is a good idea is because everything becomes what you'd expect; the component function currently returns a class that instantiates a Component class in its constructor, which is bit of a workaround. Extending Component makes more sense and it means that you'll be dealing with Components directly. Consider the following:

const TestComponent = component({
  view: () => <h1>Foo Bar</h1>
});

mount(new TestComponent()); // Why can I do new TestComponent? That's abstracted away.

class TestComponent extends Component {
  view() { return <h1>Foo Bar</h1>; }
}

mount(new TestComponent()); // This makes perfect sense since I can see that TestComponent is a class

Extending Component would also enable for a clearer actions syntax since actions could be defined on the class directly (no binding by Radi necessary!). this.someAction() would genuinely refer to the someAction method on the current class, rather than the someAction function on some this value that got bound.

component({
  view: (component) => <h1 onclick={component.someAction}>Foo</h1>,
  actions: {
    someAction() { this.foo = 'bar'; }, // 'this' is actually referring to the Component instance because it's bound
  }
});

class Foo extends Component {
  someAction() {
    this.foo = 'bar'; // 'this' is actually the Foo class
  }

  view() {
    return <h1 onclick={this.someAction}>sd</h1>; // Notice that I can use 'this' here too, because it refers to the Foo class
  }
}

Notice that it's also possible to have both a component function and extends Component, so that users have a choice.

rafaelklaessen commented 6 years ago

A schema for state wouldn't necessarily make sense because state should always be defaulted (eg, if you've got state.counter, you're setting it to 0 rather than leaving it undefined).

Marcisbee commented 6 years ago

Extending component makes perfect sense.

Agree, but schema (or how we treat it now) on state would be useful things happen the other way around - trying to set value on object that is undefined. Would also make sense for providing strict rules for that custom .map function. But this is just for sake of discussion.

rafaelklaessen commented 6 years ago

I don't think it would affect the map function on listener. How about only allowing state to be set when that piece of state was defaulted?

ambewas commented 6 years ago

component reminds me a lot of React.createClass;

Imho, it is not really necessay anymore since class became prevalent in JS. So, yeah, my vote goes entirely to extending Component.

The rest we did discuss already, I think you know I agree on these points :-)

rafaelklaessen commented 6 years ago

Yeah, just creating an issue so that everyone can share their thoughts & kind of like a mini-roadmapish.

harry-sm commented 6 years ago

My shallow proposal is purely syntactic sugar. Use decorators.

@Component()
class TestComponent {
  view() { return <h1>Foo Bar</h1>; }
}

The Action decorator is just for labeling.

@Component()
class TestComponent {
  view() { return <button onClick={this.someAction}>Click Me</button>; }

  @Action()
  someAction() { alert("I was clicked!"); }
}
jpitchardu commented 6 years ago

I do like the proposal from @harry-sm since that could give more room to composition and decoration instead of "inheritance", however I think the function API component(schema) should not be deprecated since it provides the possibility of a functional approach (I was looking for something like that with react).

harry-sm commented 6 years ago

@pichardoJ Could you elaborate on keeping the component function? Is there some advantage to it?

harry-sm commented 6 years ago
@Component()
class TestComponent {
  view() { 
    return (
        <div className="screen">{ this.message }</div>
        <button onClick={this.someAction}>Click Me</button>;
    ) 
  }

  @State()
  message = '';

  @Action()
  someAction() { this.message = "Hi, there!" }
}

Could the fucntion of l() be replaced by a @State() decorator ? Could there be typescript support? Why is there the possibility of IE8 support? don't encourage bad behavior let it die, right? 🀣

jpitchardu commented 6 years ago

@harry-sm Having a functional approach gives us the possibility of dynamic components (kinda what HOC do in react), for example

If I have a button component and I want to extend or override its functionality I would have to extend the class

@Component
class ButtonComponent {
     // Stuff
    @Action()
     onClick(){}
}

@Component 
LogButtonComponent extends ButtonComponent {
     // Stuff

     @Action
     onClick(){
         super.onClick();
         console.log("Clicked");
     }
}

:-1:

With HOC we would probably do something like

const buttonWithAdditionalAction = action => {
    return class extends React.Component {
         // Stuff
         onClick(){
              // Normal Behavior
              action();
          }
     }
}

Which is Ok

So with the component function we could just

var buttonWithAddtionalAction = action => component(
    {
        //normalProps,
        onClick: () => {
            //Normal Stuff; 
            action()
        )
    });

So I do like it way more that extending classes. I think TS support should be on the plate. And aye to that last question :+1:

rafaelklaessen commented 6 years ago

The component function literally returns a class. Everything that can be done with component could therefore be done with extends Radi.Component (including HOCs), so I don't think @pichardoJ 's argument is valid (or I just don't understand it correctly!).

harry-sm commented 6 years ago

@rafaelklaessen I think @pichardoJ simply prefers that style over extending the class. I myself prefer the syntax of extending the class. As stated by you here, extending the class is simply more explicit.

harry-sm commented 6 years ago

As proposed here,

Reuse the npm prop-types package to provide a type checking system for props.

Personally, I would prefer if typescript was used. Not only does Typescript solve the problem of type checking it also comes with other features, which I'm starting to learn about.

Separate actions, mixins, props and state. So not this.foo but this.state.foo.

I agree with the namespacing for the state. As it relates to actions, I have no issue with this.action() and see no reason presently to namespace actions. Mixins, personally single inheritance is confusing enough lol, multiple inheritances nightmare... maybe?

Add a map callback to Listener, which would be a shortcut for mapping arrays, but it would also enable for more efficient list rendering in the future.

@rafaelklaessen could you present an example, please?

jpitchardu commented 6 years ago

@rafaelklaessen @harry-sm I'm not objecting against extension nor decoration I'm up for both, I'm just saying that keeping a functional approach as well might be handy for some things that with a class sintax would require inheritance/chaining.

rafaelklaessen commented 6 years ago

@harry-sm TypeScript is fine, but the prop-types should still be included in case the end user does not want to use TypeScript.

The map function would work like this (assuming that foo is an array): l(component, 'foo').map(/* map over the array and return some jsx */);

The actions wouln't need to be namespaced when they are defined as direct methods on a components (aka as methods on the class), but as of now they're in an actions object. I propose taking them out of that actions object, which would remove the need for any action namespacing (+ it makes more sense to me).

harry-sm commented 6 years ago

@pichardoJ I hear you. however, I can't imagine a situation where I would want to use component({}). This may be due to my lack of understanding and experience

harry-sm commented 6 years ago

@rafaelklaessen I agree. I'm not a fan of using object literal in that manner. What type of params can l() accept?

rafaelklaessen commented 6 years ago

@harry-sm The l() has a signature of l(component: Component, path: string*). It takes a reference to the current component and the path you want to listen on. For example, suppose I have some component:

component({
  state: { foo: 'bar' },
  /* */
});

Then the state as of now is stored in component.foo (not component.state.foo, which I found unintuitive hence my proposal to change that). If you want to listen for updates on that state, you should do: view: (component) => <h1>{l(component, 'foo')}</h1>

jpitchardu commented 6 years ago

I do agree with @rafaelklaessen, the state should be kept in the state property and defaulting l to listen on state, maybe without needing the component arg, something like () => <h1>{l('foo')}</h1> to listen on this.state.foo.

harry-sm commented 6 years ago

@rafaelklaessen I got it. Having a map call back makes more sense now

harry-sm commented 6 years ago

@pichardoJ how would l() know that foo is a part of that specific component state? From what I'm seeing l() isn't component aware, hence you currently have to pass the component to access that component state

rafaelklaessen commented 6 years ago

@harry-sm That's correct. I was thinking about passing a state function to views that would remove the need for the component parameter since that state function would simply do something like: const state = (...path) => l(component, 'state', ...path); and that would be injected so the user won't need to reference component manually.

jpitchardu commented 6 years ago

Oh ok, yeah I see now @harry-sm, how would that be @rafaelklaessen? I mean, how would the user build a component with that state function?

harry-sm commented 6 years ago

@pichardoJ I think the end result @rafaelklaessen is going for is

view(state){
    retrun <div>{state('foo')}<div>
}

or

view(state){
    retrun <div>{state.foo}<div>
}
jpitchardu commented 6 years ago

Yeah that makes sense and it looks pretty nice actually

rafaelklaessen commented 6 years ago

@harry-sm The first one yes! :smile:

harry-sm commented 6 years ago

@pichardoJ yes it is.

harry-sm commented 6 years ago

@rafaelklaessen I like it and as much as I'm, not a fan of using string paths I will make mistakes (rubbish speller). It's an elegant solution.

harry-sm commented 6 years ago

@rafaelklaessen @pichardoJ I do have a purely theoretical idea, unsure if implementing it is even possible and not properly formed.

@Component()
class TestComponent {
  view() { 
    return (
        <div className="screen">{ this.message }</div>
        <button onClick={this.someAction}>Click Me</button>;
    ) 
  }

  @State()
  state = {
    message: '';
  }

  @Action()
  someAction = () => { this.state.message = "Hi, there!" }
}

The state decorator would connect the state to the component and also create accessors for the state properties.

const l_data;
Object.defineProperty(state, 'message', {
    get: function() {
        return l_data.getValue('message');
    },
    set: function(val) {
        state['message'] = val;
        l_data = l(component, state['message']);
    }
});

What is this solution meant to achieve?

  1. The elemiation of a this.setState() || l()
  2. The replacement of state('foo') with state.foo
view(state) { 
    return (
        <div className="screen">{ state.message }</div>
        <button onClick={this.someAction}>Click Me</button>;
    ) 
  }

 @Action()
  someAction = () => { this.state.message = "Hi, there!" }

So using accessor we get to keep on doing this.state.message = "Hi, there!" without the need of l() or and other setState function visible to the end user (me).

Creating dynamic accessors seems possible but I haven't investigated as yet. https://stackoverflow.com/a/45239384/492251

Performance Impact

I have no idea lol

harry-sm commented 6 years ago

@rafaelklaessen @pichardoJ Also, should the decision be made to keep

{
  actions: {
    someAction: () => {}
  }
}

or

@Action()
  someAction = () => { this.state.message = "Hi, there!" }

which ever variant, I have thought of one benefit, injecting it into the view along with state.

view(state, action) { 
    return (
        <div className="screen">{ state.message }</div>
        <button onClick={action.someAction}>Click Me</button>;
    ) 
}

why?

it could remove the need to do:

    <button onClick={this.someAction.bind(this)}>Click Me</button>

or

    construtor() {
        this.someAction = this.someAction.bind(this)
    }

or

    someAction = () => {}

That's one benefit I see.

rafaelklaessen commented 6 years ago

@harry-sm something with accessors should actually be possible. Radi already uses something like that internally.

Although, I would implement it like this:

// Radi currently uses a PrivateStore, let's just assume we can use it for this.
// Obviously a lot of details are missing but this should give you an idea.
Object.defineProperty(state, key, {
    get: () => l(component, 'state', key), // We return a Listener via l() because the r() JSX function will take care of rendering it, as it currently does
    set: (value) => component.privateStore.setItem(key, value)
});

The above example misses important details but it's certainly possible to implement it the way you proposed - I think.

Regarding the action object, I don't really see a problem with users having to bind it themselves (or using arrow functions), since the @Action annotator would be another layer of 'magic' & it makes sense to me to have actions being genuine methods on the class.

harry-sm commented 6 years ago

@rafaelklaessen

Object.defineProperty(state, key, {
    get: () => l(component, 'state', key),
    set: (value) => component.privateStore.setItem(key, value)
});

I wouldn't return the listener I would return the value. Remember state is accessible throughout the component, not just in the view(). Let's say we have a case where we need the state value to perform some operation and produce some output.

@Component()
class TestComponent {
  view() { 
    return (
        <div className="screen">{ this.state.val }</div>
        <button onClick={this.someOp}>Click Me</button>;
    ) 
  }

  @State()
  state = {
    val: 0;
  }

  someOp = () => { 
    const { val } = this.state;
    this.state.val = val > 10 ? 
        val * 1.1 : val + 1 * 1.05 
  }
}

In the above case returning this listener would not give us access to the data we need. The idea of using accessors is to simplify things for the user. If the user assigns this.state.val = 1 when the user retrieves the value they expect 1 and not an object/ function.

What we want to maintain is the simplicity of the API. Remember when you started to code, the first thing you learned, maybe was variable declaration, initialization, and assignment. var a = 1; console.log(a) // 1

That feels natural and that is the objective of assigning values to the state. this.state.a = 1 console.log(this.state.a) // 1

We are hiding the complexity and usage of l(). The user will never know l(). There are no bells and whistles like l() or this.setState, no additional procedure to remember. Using Radi in this regard should feel like basic coding. The simplicity is what makes it alluring and makes you marvel.

rafaelklaessen commented 6 years ago

@harry-sm That would indeed be the most optimal way, but how can you update DOM nodes when there is no way to tell whether some value is going to be changed? Remember that Radi doesn't have VDom. The only things that Radi ever updates are values that are wrapped in Listeners, because that's the most efficient way.

The API must provide the user with something that gives them the ability to wrap values in Listeners. Otherwise all values are static.

Marcisbee commented 6 years ago

@harry-sm We should be returning Listener to DOM. Otherwise there is no point for us to use l() as we no longer know what is changing and where it is changing in DOM.

Also this Listener vs real data usage needs to be conscious. User needs to be aware if he is manipulating with Listener or actual state.

And Listener needs to be aware of component it should be listening to.

Beauty of current system is that we can listen to data in another component. What we should be considering is how to make things clear for user.

"Oh, I am using static data":

<div>{ this.state.number }</div>

"Now this doesn't look like actual data to me":

<div>{ this.listen("number") }</div>
<div>{ anotherComponent.listen("number") }</div>

If we try to hide things, then we can run into infinite amount of problems.

this.number - static data or listener ? this.state.number - static data or listener ?

rafaelklaessen commented 6 years ago

I agree with @Marcisbee. I like the this.listen thing btw, is that already implemented or a proposal? Either way, how do you listen on a state object with it? this.listen('state', 'number')? If so, this.listenState('number') might be a useful shortcut since users will practically always listen on state and nothing else.

We could still have the state function like I proposed as well. I don't think listening on other components will be used a lot.

harry-sm commented 6 years ago

@Marcisbee Welcome lol. Everything on state is a listener. That's why we use a @State() decorator to transform state objects into listeners.

Yes returning the listener to the dom is needed I see that now. Could we have then

 @State()
  state = {
    foo: 0;
  }

  someOp = () => { 
    this.state.foo; \\ listener.
    this.state.foo.value \\ actual static value.
    console.log(this.state.foo.value);
  }

anotherComponent.listen("number") Having another component listen for another component state, changes the relationship among them, I do like the one way flow of data approach. ComponentY listens to ComponentX, ComponentX listens to ComponentZ and ComponentX listens ComponentY. Not my cup of tea.

harry-sm commented 6 years ago

@rafaelklaessen How can we avoid string paths lol?

rafaelklaessen commented 6 years ago

@harry-sm Well if this.state.foo is a Listener you could just do <h1>{this.state.foo}</h1>.

harry-sm commented 6 years ago

@rafaelklaessen some cases I want the value of foo the actual value.

@Component()
class TestComponent {
  view() { 
    return (
        <div className="screen">{ this.state.val }</div>
        <button onClick={this.someOp}>Click Me</button>;
    ) 
  }

  @State()
  state = {
    val: 0;
  }

  someOp = () => { 
    const { val } = this.state;
    this.state.val = val > 10 ? 
        val * 1.1 : val + 1 * 1.05 
  }
}
rafaelklaessen commented 6 years ago

I understand. You can do this.state.foo.value in such cases, like you said. I meant that with this.state.foo being a Listener, you don't have to use string paths.

harry-sm commented 6 years ago

@rafaelklaessen Ok and we still get to do this.state.foo = 2 instead of a this.setState({foo: 2}) scenario.

What do you think about listening to other components state via another component?

rafaelklaessen commented 6 years ago

Correct. Also, I'm not a big fan of listening on other components, but I'm not against it either.

harry-sm commented 6 years ago

@rafaelklaessen ok. What about using typescript?

jpitchardu commented 6 years ago

@Marcisbee I like the idea of this.listen('path') and I agree with @rafaelklaessen the state properties should be listeners and using the decorator we could transform them.

I do have a concern about listening to another component's state as I think it can turn up to be like the $scope mess in angularjs, so I do believe every component should have an isolated state and notify other components through events or method properties.

jpitchardu commented 6 years ago

And about state change I do like the idea of this.state.foo = val instead of this.setState({... this.state, foo: val}) as it seems to me that radi is based on micro-mutations on the DOM instead of checking it all again for changes. The only issue with that would be immutability, any ideas on how to manage that?

jpitchardu commented 6 years ago

@harry-sm I don't think typescript should be the main language, however I do consider that support for ts should be provided, mainly through typings.

rafaelklaessen commented 6 years ago

@pichardoJ I understand your concern. However, in React you could also call setState on another component if you'd have a reference to it; as well as any lifecycle methods. Yet, I doubt that anyone ever does so.

Listening on other components is not a feature, yet it is something that shouldn't be purposely blocked.

harry-sm commented 6 years ago

@pichardoJ I do like the idea of typescript being the main language. I do understand that it would add to the size of the code base when compiled. Also, I don't think there is a polyfill for decorators. I'm not against typings but I fear the code implementation and the type declarations might at some point go out of sync which I deeply fear even though it's a small issue. I am in favor of a typescript first approach. there is no real reason not to use typescript in my mind.