Skip to content

migrate ngOutlet to typescript#4386

Closed
shahata wants to merge 3 commits into
angular:masterfrom
shahata:tsRouter
Closed

migrate ngOutlet to typescript#4386
shahata wants to merge 3 commits into
angular:masterfrom
shahata:tsRouter

Conversation

@shahata

@shahata shahata commented Sep 27, 2015

Copy link
Copy Markdown

@btford plz review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't it be preferable to create this class once (outside of the link function), instead of recreating it ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It would, but since we need a lot of things from the closure (scope, element, transclude, etc.) I prefer not having to pass all of those in the constructor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer not having to pass all of those in the constructor

Any particular reason ?

Besides the perf impact of creating a new function in each outletLink fn execution, it is also hard to spot what outer dependencies the class has.
Since it's a "non-public" class (i.e. we don't care about breaking the API if needed), why not pass the arguments explicitly ?

@tbosch tbosch added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 28, 2015
@btford btford added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 29, 2015
@btford btford assigned shahata and unassigned btford Sep 29, 2015
@btford

btford commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

@shahata – can you follow up on my and @gdi2290's comments?

Other than that, looks good! 👍

@shahata

shahata commented Sep 29, 2015

Copy link
Copy Markdown
Author

@btford @gkalpak Fixed everything except for defining directive introspection on ng module which I think is legitimate (though test is missing) and moving class outside of closure (see comment)

@btford

btford commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

LGTM, merging.

@mary-poppins

Copy link
Copy Markdown

Merging PR #4386 on behalf of @btford to branch presubmit-btford-pr-4386.

@btford btford added the action: merge The PR is ready for merge by the caretaker label Sep 30, 2015
@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 30, 2015
@btford btford assigned btford and unassigned shahata Sep 30, 2015
@btford btford removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 30, 2015
@btford

btford commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

Hm, this never got merged. Let me rebase and try again.

@btford

btford commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

Pushed to presubmit again after some rebasing: https://travis-ci.org/angular/angular/builds/84430878

👍

@shahata shahata closed this in 31f48ae Oct 9, 2015
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants