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

Makes sure dependecies are loaded on demand #6072

Merged
merged 1 commit into from Jul 14, 2016
Merged

Makes sure dependecies are loaded on demand #6072

merged 1 commit into from Jul 14, 2016

Conversation

twokul
Copy link
Contributor

@twokul twokul commented Jul 14, 2016

results: https://gist.github.com/twokul/54542ed6783e20bbfd2f62bd9dfb89c7

before:

Total require(): 1375
Total time: 1.1s

after:

Total require(): 995
Total time: 832ms

depends on #6071
related to #6009

@nathanhammond
Copy link
Contributor

nathanhammond commented Jul 14, 2016

LGTM assuming all tests pass. Has the tradeoff of making it a bit less clear which dependencies are pulled into any particular "module."

@Turbo87
Copy link
Member

Turbo87 commented Jul 14, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Jul 14, 2016

📌 Commit a9042f3 has been approved by Turbo87

homu added a commit that referenced this pull request Jul 14, 2016
Makes sure dependecies are loaded on demand

results: https://gist.github.com/twokul/54542ed6783e20bbfd2f62bd9dfb89c7

before:

```
Total require(): 1375
Total time: 1.1s
```

after:

```
Total require(): 995
Total time: 832ms
```

depends on #6071
related to #6009
@homu
Copy link
Contributor

homu commented Jul 14, 2016

⌛ Testing commit a9042f3 with merge 23612ad...

@Turbo87 Turbo87 added this to the v2.8.0 milestone Jul 14, 2016
@homu
Copy link
Contributor

homu commented Jul 14, 2016

☀️ Test successful - status

@homu homu merged commit a9042f3 into master Jul 14, 2016
@Turbo87 Turbo87 deleted the lazy-deps branch July 14, 2016 09:28
@Turbo87
Copy link
Member

Turbo87 commented Jul 14, 2016

@twokul could I get you to use branches in a personal fork for PRs instead of branches in this repo? all the PR branches will get cloned too when someone forks this repo which looks a bit unclean and also makes Travis run the tests twice. @stefanpenner is an exception here because of the repo redirection rules that apply as long as he doesn't fork this repo again.

@rwjblue
Copy link
Member

rwjblue commented Jul 14, 2016

@twokul - What command were you running for the benchmark times? I see in the gist that you include the output, but not the command you were executing. I'm guessing it was ember version?

@twokul
Copy link
Contributor Author

twokul commented Jul 14, 2016

@Turbo87 sure thing, man. my bad

@rwjblue it is ember v

@twokul
Copy link
Contributor Author

twokul commented Jul 14, 2016

@Turbo87 should we cherry pick this commit into minor release, 2.6.3, along with #6068?

@nathanhammond
Copy link
Contributor

@twokul If you're doing a release with the event emitter, go ahead.

@Turbo87 Turbo87 modified the milestones: v2.7.0, v2.8.0 Jul 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants