Skip to content

removes extra info for Ember.run wrapping #192

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 1, 2018

Conversation

gokatz
Copy link
Contributor

@gokatz gokatz commented Oct 6, 2018

related to #187

@gokatz
Copy link
Contributor Author

gokatz commented Oct 6, 2018

@jenweber
Copy link
Contributor

jenweber commented Oct 9, 2018

Hey @sivakumar-kailasam can you take a look at this PR and let me know your thoughts on removing vs replacing this material?

@jenweber
Copy link
Contributor

jenweber commented Oct 9, 2018

Hi! This PR was still open as of creating the 3.5 release. This means before this PR to be merged, any changes requested should be made, and then the same additions should be applied to the upcoming 3.5 version files. They will be available later this week. Sorry about the extra step!

@gokatz
Copy link
Contributor Author

gokatz commented Oct 9, 2018

No issues. will accommodate these changes to the newer versions too.

Copy link
Member

@sivakumar-kailasam sivakumar-kailasam left a comment

Choose a reason for hiding this comment

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

@gokatz can we restore the sections until 3.3? Afaik backburner got updated in 3.4 which brought in the microtask changes that made it easier for devs to no longer worry about how run loop works under the hood.

So lets remove those sections only for 3.4 & 3.5. Also could you rebase against master & make the changes in 3.5 as well?

@gokatz
Copy link
Contributor Author

gokatz commented Oct 11, 2018

But, when I tested in a 2.18 app, I don't get any run loop assertion during the test. Am I missing anything?

@sivakumar-kailasam
Copy link
Member

afaict, BackburnerJS/backburner.js#306 got merged early in feb & released as 2.2.0. It made its way into ember in 3.2.0. So unsure how that'd work on a prior version ¯_(ツ)_/¯

@gokatz
Copy link
Contributor Author

gokatz commented Oct 11, 2018

will re-check and update

@jenweber
Copy link
Contributor

Hi @gokatz, I've chatted with a couple people like Mike North and have learned that the run guides are more useful than we thought. Could you take a second pass at this and see what sections we can salvage that explain the run loop, but maybe without the part where it makes it look like people should wrap code in a run?

@gokatz
Copy link
Contributor Author

gokatz commented Oct 28, 2018

Hi @jenweber, Sorry for the delay 😬
Yes. I think we can only trim off the section that talks about the issue in testing (autorun assertion).

Also, we mentioned autoruns are suboptimal. Can we rephrase those as per this RFC or I'm missing anything?

@jenweber jenweber dismissed sivakumar-kailasam’s stale review December 1, 2018 23:27

Requested changes have been made

@jenweber jenweber merged commit eed6c36 into ember-learn:master Dec 1, 2018
@jenweber
Copy link
Contributor

jenweber commented Dec 1, 2018

Thank you @gokatz! Merged!

@jenweber
Copy link
Contributor

jenweber commented Dec 1, 2018

Also @gokatz I do think it would be good to refactor how we talk about runs. We should check to see what from that RFC has actually been implemented.

@gokatz
Copy link
Contributor Author

gokatz commented Dec 2, 2018

Thanks @jenweber, should we track it somewhere?

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

Successfully merging this pull request may close these issues.

3 participants