react-adopt

react-adopt copied to clipboard

Given the following code snippet from the readme:

const Composed = adopt({
  cart: <Cart />,
  user: <User />,
  shippingRates: ({ user, cart, render }) => (
    <ShippingRate zipcode={user.zipcode} items={cart.items}>
      {render}
    </ShippingRate>
  )
})

How do you know in which order to apply the render prop components? Does this rely on the order of keys returned from Object.keys(mapper) call here?

As far as I know, object property order is not guaranteed. Asking because I'd love to use this library, but wondering if this could lead to some nasty (maybe cross browser) bugs.

Example of what could go wrong

const First = ({ children }: FirstProps) => children("foo");
const Second = ({ first, children }: SecondProps) => children(`${first}bar`);
const Display = ({ first, second }: DisplayProps) => (
  <div>
    First: {first}
    <br />
    Second: {second}
  </div>
);

const Composed = adopt({
  first: <First />,
  second: ({ first, render }) => <Second first={first}>{render}</Second>
});

const ComposedInverse = adopt({
  second: ({ first, render }) => <Second first={first}>{render}</Second>
  first: <First />,
});

const App = () => (
  <div style={styles}>
    <h3>Correct order</h3>
    <Composed>
      {({ first, second }) => <Display first={first} second={second} />}
    </Composed>

    <h3>Inversed</h3>
    <ComposedInverse>
      {({ first, second }) => <Display first={first} second={second} />}
    </ComposedInverse>
  </div>
);

Result

This is indeed a problem.

Do you plan on implementing something that guarantees the order of execution?

I guess you could think of an API where passing an object does not guarantee order execution, but passing an array does :man_shrugging:

An alternative would be to mention this caveat in the readme, referring to this SO answer, which outlines the cases for ES2015 nicely:

This results in the following order (in certain cases):

Object {
  0: 0,
  1: "1",
  2: "2",
  b: "b",
  a: "a",
  Symbol(): "sym"
}

integer-like keys in ascending order normal keys in insertion order Symbols in insertion order

The question is, for what methods this order is guaranteed in the ES2015 spec?

The following methods guarantee the order shown:

Object.assign Object.defineProperties Object.getOwnPropertyNames Object.getOwnPropertySymbols Reflect.ownKeys The following methods/loops guarantee no order at all:

Object.keys for..in JSON.parse JSON.stringify

From the source code, I see that this lib uses Object.keys, which according to the above post does not guarantee any order at all :cry:

@pedronauck any opinion on that?

I see that @lucasconstantino, sorry the too late response... but unfortunately never came to my mind about that so far, I tried quickly to see something using Object.entries without breaking current api but I didn't get much success and I didn't have to much time at all.

How do you now, adopt() use keys to iterate then reduce() the object. So, each iteration after the first one, the previous result is passed as a prop for render method. If you do something like that:

const ComposedInverse = adopt({
  second: ({ first, render }) => <Second first={first}>{render}</Second>
  first: <First />,
});

... you're assuming that first is a previous result from some key called foo. According to the current implementation this will fail, because second will iterate before foo.

To be honest, I didn't have any case that I create some object and the iteration order fails because was as iteration using keys(), or the second prop ran before the first one, even with a value that the result is async.

But I'm open to hear more about which solution do you think that can fit here!

To be honest, I didn't have any case that I create some object and the iteration order fails because was as iteration using keys(), or the second prop ran before the first one, even with a value that the result is async.

Yeah, so it seems that "most common cases" work for "most"/modern browsers. This would explain why you haven't run into it yet. However, because the order of iteration is not defined in the language spec, there is no guarantee that browsers will stick to the logic they currently execute. Nor is there a guarantee that every browser currently supports iterating in the order of insertion.

Plainly said; I see two issues here:

1. There might be some obscure browser that we are not personally testing with (say IE6), for which the order of iteration is different in some edge case. If this is the case, these are the bugs that cost a lot of time to track down, if they are tracked down at all :disappointed:
2. Because there is no spec about the order, even Chrome or Firefox could change their behavior, which could break this library at any point in the future!

If you want to be sure about the order, I think the "correct" way of implementing that in the library would be to accept an array, which makes the order explicit:

// This breaks, but it's predictable; the order will *always* be inversed
const ComposedInverse = adopt([
  ({ first, render }) => <Second first={first}>{render}</Second>,
  <First />,
});

// This works, always
const Composed = adopt([
  <First />,
  ({ first, render }) => <Second first={first}>{render}</Second>,
});

// Object api:

// This breaks, most of the time; depending on browser implementation
const ComposedInverse = adopt({
  second: ({ first, render }) => <Second first={first}>{render}</Second>
  first: <First />,
});

// This works, most of the time; depending on browser implementation
const Composed = adopt({
  first: <First />,
  second: ({ first, render }) => <Second first={first}>{render}</Second>
});

@edorivai the issue here is that you can't infere the prop name to be used when using an array as API.

1. Sure, indeed this is a really good point, but I think that is a trade-off, using an array how @lucasconstantino say is very hard do infere the prop name, we should do something like that:

const Composed = adopt([
  { foo: <Foo /> },
  { bar: <Bar /> },
])

and IMHO this is bad and will a unnecessary breaking change!

2. Lead with old browsers is something to care about, but make a breaking change or change the entire api based on a legacy browser problem, I think that can be a little over thinking.

I'll close this issue because I think that now this is not a "true real problem", but I'm appreciate your concern about that @edorivai and I'll track this :v:

@pedronauck I think a neat solution would be to create a true Map using the object data input, and transforming it as it is done today to have the order based on the object's key. If we do that, we can easily move to accept both Maps or plain objects as argument to the adopt method, leaving the user to care about using a Map if he is facing browser issues.

My point: this would be no breaking change, and would create a viable path for those who need to care for this.

[EDIT] Maybe we could flag this issue with low priority? If someday someone decided to work on a pull-request, fine.

@lucasconstantino Yeah, the Map solution would be quite nice. I agree that in most (dare I say > 99%) cases, this will not be an issue, and in those cases where it works the existing API is really nice!

I was bringing it up, because I foresee that when it becomes a problem, the problem will cause very nasty bugs:

1. They will only happen in certain browsers
2. They might happen only with certain uses of property names, like names starting with a number
3. They might happen in future versions of browsers :fearful:

In my experience, these are the types of bugs that will cause a developer to pull out his hairs for an entire day, if not more :joy:. Just wanted to give you guys a heads up, and perhaps give future Googlers a chance to find this issue :smile:.

Good idea @lucasconstantino, work with a Map can be a solution, I will reopen the issue and give it a low priority label!

@edorivai thanks for the heads up, it is very much appreciated. This lib reached a point where it should definitely address these uncommon but very relevant problems, if not by fixing them, at least providing people with information for the future ;)

I actually wanted to take a stab at this, only to find out that Typescript doesn't support Map out of the box.

Meaning we have 3 options:

1. Ship a polyfill along with this library on npm
2. Target ES6
3. Implement it with something other than Map

What do you guys think?