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

extendTimeout on sticky message #254

Merged
merged 2 commits into from
Dec 7, 2017
Merged

extendTimeout on sticky message #254

merged 2 commits into from
Dec 7, 2017

Conversation

Dhaulagiri
Copy link
Collaborator

No description provided.

@Dhaulagiri
Copy link
Collaborator Author

Dhaulagiri commented Nov 27, 2017

@sbatson5 I ended up reverting #240 because when reviewing I overlooked the fact that adding ember-concurrency required that dependency to be included in dependencies and not devDependencies. When I pulled 1.6.0 into our app it broke looking for ember-concurrency. Given this new information I'm wondering if there is a way to accomplish what you are doing here without ember-currency? Requiring consumers of ember-cli-flash to include it if they aren't already using it (as we are not) quadruples the size of this addon which is something I'm hesitant to do.

@sbatson5
Copy link
Collaborator

Ah, that's why I didn't see it when we pulled it into our existing app (we already had concurrency).

We can find another solution with the run loop, it will just require more cleanup when destroying the component.

@sbatson5
Copy link
Collaborator

sbatson5 commented Dec 1, 2017

@Dhaulagiri Updated this PR to use the run loop instead of concurrent tasks. I've tested it a few times and everything seems to be working as expected. Would you be able to pull it into a project and double check?

@@ -50,6 +50,7 @@
"ember-disable-prototype-extensions": "^1.1.2",
"ember-export-application-global": "^2.0.0",
"ember-load-initializers": "^1.0.0",
"ember-maybe-import-regenerator": "^0.1.6",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only used for tests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Was getting it for free with ember-concurrency apparently. Shouldn't add to the library size as it's just a devDependency but I can roll the tests back to nested andThen's if that's preferred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that sounds great to me. Just wanted to double check.

@Dhaulagiri
Copy link
Collaborator Author

Worked great in our app 👏

@Dhaulagiri
Copy link
Collaborator Author

:shipit:

@sbatson5 sbatson5 merged commit 85f664a into master Dec 7, 2017
@sbatson5 sbatson5 deleted the revert-253-br-revert branch December 7, 2017 19:43
@sbatson5
Copy link
Collaborator

sbatson5 commented Dec 7, 2017

Merged! Think we could cut another release?

@Dhaulagiri
Copy link
Collaborator Author

Released in 1.6.2

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

Successfully merging this pull request may close these issues.

2 participants