-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix deletion of additional emails #31345
Conversation
61c3f73
to
0e65613
Compare
/backport to stable23 |
/backport to stable22 |
e33efcd
to
06cc34f
Compare
Now uses https://lodash.com/docs/4.17.15#uniqueId to generate the unique key |
/compile amend / |
06cc34f
to
cbf54fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now uses lodash.com/docs/4.17.15#uniqueId to generate the unique key
I don't like the idea of using lodash. Relying on external libraries for things as simple as this isn't really the most efficient move I would say :)
Just doing any Math.random().toString(36).substring(2)
like you did is enough imho.
Importing uniqueId doesn't adds much (82B vs 39B), but it's just a good pattern I thik. In the end we should move away from lodash, jQuery and other "tooling" libs that vanilla js can do noaways 😉
I thought it was already imported as we used it in other places. Let's not import it then. My worry was more that more frontend developers will invent their own key algorithms in various places (with a certain risk of collisions), so perhaps we could add a method in the "OC." namespace that does the equivalent of what lodash does, so that every app dev can use it. |
(the latter comment was provided to you by someone who's used to chase ghosts, aka bugs that seldom happen and still caused nasty issues, so I'm usually trying to be more careful about probabilities as I tend to be unlucky there) |
cbf54fc
to
03aba3f
Compare
Yes I did
This is a valid concern but as stated above by @skjnldsv something as simple as unique id generation without any business logic specific requirements does not warrant another abstraction. Whether we make a shared unique id generation method or not is outside the scope of this specific PR and any discussions should be started elsewhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@Pytal please resolve the conflicts, not sure if rebasing is necessary considering possible recent changes on master |
Signed-off-by: Christopher Ng <chrng8@gmail.com>
03aba3f
to
2948c5e
Compare
/backport to stable24 |
|
|
Fix #31164
Using index as the key would cause data from a deleted email to persist into the additional email at the same index due to the component being reused, this is resolved by forcing component replacement with unique string keys