feat(service): handle flash object preventDuplicates similar to global option #282
Conversation
addon/services/flash-messages.js
Outdated
| @@ -140,7 +142,8 @@ export default Service.extend({ | |||
| }, | |||
|
|
|||
| _enqueue(flashInstance) { | |||
| const preventDuplicates = get(this, 'defaultPreventDuplicates'); | |||
| const preventDuplicates = get(this, 'defaultPreventDuplicates') || | |||
| get(flashInstance, 'preventDuplicates'); | |||
There was a problem hiding this comment.
I'd have imagined that the option on the flashInstance itself should trump the global setting as it's more specific. This would not be the case with the above logic. What's the intention?
There was a problem hiding this comment.
You're right, that makes more sense.
I'll change it soon
|
I've made some changes so it'll always prefer the instance option over the global one. |
| @@ -30,6 +31,10 @@ export function guidFor(dependentKey) { | |||
| get() { | |||
| const value = get(this, dependentKey); | |||
|
|
|||
| // it's possible that a value has no toString as some objects don't implement the guid field | |||
| // this early return it to avoid errors being thrown when calling undefined.toString() | |||
| if (isNone(value)) return; | |||
There was a problem hiding this comment.
Perhaps just checking if (typeof value.toString !== "function") would show your intention more obviously?
There was a problem hiding this comment.
Sadly it would require additional checks as typeof undefined.toString will throw an error:
Uncaught TypeError: Cannot read property 'toString' of undefine
makepanic
force-pushed
the
issues/281
branch
from
a14d327 to
f7c563a
Compare
|
I've rebased this PR onto the current master |
makepanic
force-pushed
the
issues/281
branch
from
f7c563a to
9d12a42
Compare
|
@sbatson5 @abhilashlr do you think we could also merge/work on this PR? We used it since 2018 in production and haven't seen any issues. If so, I'll check if I can find anything to improve. Right now it's only rebased onto the current master. |
|
@sbatson5 Seems like the PR has test cases passing. If this is fine, I think we should merge it? |
|
Would really love to see this capability. |
|
Is there anything specific which is preventing this from being merged? |
|
Rebased it onto the current master |
fixes #281
Everything open for discussion :)