-
Notifications
You must be signed in to change notification settings - Fork 56
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
Question: do "previous mapper results" rely on order of keys #10
Comments
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 🤷♂️ An alternative would be to mention this caveat in the readme, referring to this SO answer, which outlines the cases for ES2015 nicely:
From the source code, I see that this lib uses |
@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 How do you now,
... you're assuming that To be honest, I didn't have any case that I create some object and the iteration order fails because was as iteration using But I'm open to hear more about which solution do you think that can fit here! |
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:
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. |
const Composed = adopt([
{ foo: <Foo /> },
{ bar: <Bar /> },
]) and IMHO this is bad and will a unnecessary breaking change!
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 ✌️ |
@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 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 I was bringing it up, because I foresee that when it becomes a problem, the problem will cause very nasty bugs:
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 😂. Just wanted to give you guys a heads up, and perhaps give future Googlers a chance to find this issue 😄. |
Good idea @lucasconstantino, work with a |
@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:
What do you guys think? |
Given the following code snippet from the readme:
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
Result
The text was updated successfully, but these errors were encountered: