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

deprecate define_state (?) #89

Open
catmando opened this issue Nov 30, 2015 · 4 comments
Open

deprecate define_state (?) #89

catmando opened this issue Nov 30, 2015 · 4 comments

Comments

@catmando
Copy link
Collaborator

now that implicit state construction has been (tentatively) added, should we deprecate define_state?

I believe so, why? Because it makes the connection between "state variables" and "instance variables" clear. You don't explicitly declare instance variables, so why should you declare state variables?

Good style practice then would be to initialize all state variables (like you would instance variables) in the before_mount macro.

More important however is how state variables get initialized. The current define_state macro allows initializing but its very confusing, as the initial value is computed once per class, NOT per instance as you would expect. That is why I believe that states should be "created" within the before_mount macro, because that allows the state to be computed on a per instance basis (just like normal instance variables)

consider this code:

class MyComponent < React::Component::Base
  param :users, type: [User]
  before_mount do
    state.users! params.users
    state.mount_time! Time.now
  end
  ...

if good style insists that all state variables are initialized in the block (as above)
then there is no redundancy.

On the other hand if we determine that good style uses define_state, then you end up with redundant code.

Moreover the above makes its much clearer how and when the state is initialized.

@Thermatix
Copy link

I like it like this, I tend to think of the before_mount macro as the components "constructor". So anything that needs to run on creation gets placed here.

@ajjahn
Copy link
Collaborator

ajjahn commented Dec 8, 2015

State declaration should be required.

If the motivation behind this change is to reduce keystrokes and 'DRY' up lines of code, I think it violates DRY. DRY principles don't exist to reduce typing. They're intent is to reduce duplicated knowledge.
Declaring an object has state, and declaring what that state's value is are two separate bits of knowledge.

Declaring state self documents and provides expressive shared understanding by convention about the given component and the attributes that influence it's behavior. We shouldn't assume that a line of code is only valuable if it's telling the computer something. I'd argue code is equally if not more valuable when it tells a human something.

Removing the state declaration requirement in favor of using the before_mount block makes it pretty easy to skip initializing state altogether. I find this problematic because it is particularly hard to reason about state that at some point in time did not exist, then suddenly at another point in time, has come into existence with some value.

Imagine a light bulb that doesn't have an on/off state. Now imagine I just turned the light bulb on. A confused programmer's inner monologue might go something like this:

"Wait, what? Just a second ago it had no on/off state."
"Well now it does and it's on."
"What was the on/off state before then?"
"It must have been off."
"So it had state all along?"
"I guess so."

Not declaring state should be the same as directly declaring no state, not declaring optional state.

The current define_state macro allows initializing but its very confusing, as the initial value is computed once per class, NOT per instance as you would expect.

This problem should be solved by changing the implementation to initialize state at component instantiation, not class declaration. This would be clean and intuitive and leave the before_mount callback for cases when additional up front processing is needed.

@ajjahn
Copy link
Collaborator

ajjahn commented Dec 10, 2015

More on this issue in #97 (comment).

@sollycatprint
Copy link

This issue was moved to ruby-hyperloop/hyper-react#89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants