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

General usage feedback (naming things, type checks) #365

Open
micahjon opened this issue May 6, 2019 · 4 comments
Open

General usage feedback (naming things, type checks) #365

micahjon opened this issue May 6, 2019 · 4 comments
Labels

Comments

@micahjon
Copy link
Contributor

micahjon commented May 6, 2019

Last month I launched a silly, React-based side project with Microstates (anymammalsmilk.com) and overall had a great experience with it. Thanks in particular for your help with issue #342, which was resolved in only 9 hours!

Figured the least I could do was share some feedback as a Microstates newcomer that might make it easier to use for other folks down the road.

How to distinguish Microstate variable names from the values they represent?

For instance, I might pass a "votes" Microstate as a prop to a React component that calls valueOf and further transforms the resulting Array of votes. If "votes" is the Array, then what should I call the prop? If "votes" is the prop, how can I tell it apart from other, non-Microstate props?

In jQuery-land, this issue was solved by adding a $ to the beginning of jQuery elements (e.g. $pageHeader) to distinguish them from standard DOM Elements (e.g. pageHeader). Having a convention would make it easier to add Microstates to an existing codebase (with standard Arrays, Objects, etc.) and distinguish them from unwrapped values at a glance.

Would it make sense to enforce initial Microstate types?

When working within class methods, I would occasionally forget to unwrap a nested Microstate before setting it on a property. For instance:

class Person {
  constructor() {
    this.name = String;
    this.age = Number;
  }
  updateName(name) {
    return this.set({ name, age: this.age });
  }
}

let person1 = create(Person, { name: "Homer", age: 39 });
let person2 = person1.updateName("Micah");
// person2.age is now a Microstate object, not 39

In the (contrived) example above, it feels natural to set age to this.age, but we really need to use { age: valueOf(this.age) } or age will be set to an object instead of a number.

It might be out of the scope of this library, but it would be helpful if an error was thrown when values of incorrect types are set, so that programmer errors like this are more quickly fixed. Again, probably out of scope but it feels strange to be able to get away with this:

let num1 = create(Number, 5);
let num2 = num1.set("random string");

Just my two cents. Thanks for a great library, and happy to discuss any of this further!

@taras
Copy link
Member

taras commented May 6, 2019

@pranksinatra thank you for sharing your thoughts and I'm glad you're enjoying using Microstates.

it would be helpful if an error was thrown when values of incorrect types are set

We need to do a run at improving developer feedback in general.

This can be one of the areas that we'll improve. We need to make a decision wether we want to surface this kind of feedback in this library or delegate to TypeScript. In the past, we discussed adding something equivalent to propTypes but for transition arguments. This could be another option.

For what it's worth, you might find it useful to use the chaining syntax that would transparently deal with this problem. For example, you could say

class Person {
  constructor() {
    this.name = String;
    this.age = Number;
  }
  updateName(name) {
    return this
     .name.set(name)
     .age.set(this.age);
  }
}

Using chaining does 2 things that can be helpful,

  1. this.age microstate will get unwrapped and it's equivalent to saying this.age.set(valueOf(this.age))
  2. microstates will automatically debounce no-op transitions

Is it possible to see the code of you application? I'm curious to see how you use transitions.

@cowboyd
Copy link
Member

cowboyd commented May 6, 2019

let num1 = create(Number, 5);
let num2 = num1.set("random string");

Microstates is supposed to coerce a value into an acceptable representation, and you've actually uncovered a bug in the NumberType in your example:

create(Number, 'hello')

should return a Microstate with value NaN because 'hello' is not a number.

It turns out though that there is a bug here and isNaN returns true not just of the value NaN, but for anything that cannot be converted to a number, so isNaN('hi') //=> true

This initializer should more correctly written as:

  initialize(value) {
    if (value == null) {
      return 0;
    } else if (typeof value === 'number') {
      return this;
    } else if (value instanced Number) {
      return value.valueOf();
    } else {
      return Number(value);
    }
  }

@micahjon
Copy link
Contributor Author

micahjon commented May 6, 2019

@taras , here's the actual update method where I forgot to call valueOf on this.votes before setting it back on the parent Microstate.

 /**
   * Update user upon app startup, login or logout
   * @param {object/null} user
   * @param {array} votes
   */
  update(user, votes = valueOf(this.votes)) {
    user = user || {};
    const { displayName = '', email = '', photoURL = '' } = user;
    const newUserData = {
      id: user.uid || user.id || getAnonymousUserId(),
      displayName,
      email,
      photoURL,
      votes,
    };
    setLocalUser(newUserData);
    return this.set(newUserData);
  }

Full source: https://github.com/pranksinatra/any-mammals-milk/blob/master/react-client/src/lib/model.js#L27

I think I was instinctively trying to avoid intermediary transitions and any consequential re-renders in React, not realizing that Microstates already debounces these.

@taras
Copy link
Member

taras commented May 12, 2019

In jQuery-land, this issue was solved by adding a $ to the beginning of jQuery elements (e.g. $pageHeader) to distinguish them from standard DOM Elements (e.g. pageHeader). Having a convention would make it easier to add Microstates to an existing codebase (with standard Arrays, Objects, etc.) and distinguish them from unwrapped values at a glance.

I like this idea. Observables use $ at the end of the variable like this: value$. What could a good convention for Microstates be?

avoid intermediary transitions and any consequential re-renders in React, not realizing that Microstates already debounces these.

They work more like batch transitions - multiple transitions inside of method results in a single transition.

ProTip: you can return POJOs from transitions. Microstates will try to ensure that structural sharing by reusing any microstates in your POJOs - effectively doing what you suggested. So you can do this,

class Person {
  constructor() {
    this.name = String;
    this.age = Number;
  }
  updateName(name) {
    return { name, age: this.age };
  }
}

Currently, return { name, age: this.age } !== return this.set({ name, age: this.age }} maybe they should be?

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

No branches or pull requests

3 participants