fix(compiler): properly type `forwardRef` and `resolveForwardRef` by NickClark · Pull Request #11813 · angular/angular

NickClark · 2016-09-21T23:43:11Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
#11638

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

Makes forwardRef and resolveForwardRef return more meaningful types. Now when resolving a forward reference, TypeScript knows what type the returned reference is.

Closes #11638

vicb · 2016-09-22T06:21:24Z

vicb · 2016-09-22T06:26:27Z

q: Is the cast really necessary here ?

I thought it was. I was getting the error: SELECT_MULTIPLE_VALUE_ACCESSOR' has or is using name 'ForwardRefFn' from external module "/Users/nick/Code/k3/angular/modules/@angular/core/src/di/forward_ref" but cannot be named.. But just now, going back I realize now all I needed was to import the forwardRefFn. 😶

vicb · 2016-09-22T16:00:05Z

Q: I don't see how this type relates to ForwardRef, what about renaming this to Factory<T> ?

WDYT

Do we need this type at all ?

No. I just get carried away with naming my types. I was just trying to hint at it's purposed, since it's really just a function that we decorate, and wanted to clearly differentiate it from the type that is returned.

Shall I just change the forwardRef to this: function forwardRef<T>(forwardRefFnSrc: () => T): ForwardRefFn<T>? That definitely seems clear enough 😄

vicb · 2016-09-22T16:02:04Z

One minor question, otherwise LGTM

vicb · 2016-09-22T18:41:30Z

return _isForwardRefFn(type) ? type() : type;

ternary: ✅

vicb · 2016-09-22T18:42:31Z

do we need && t.hasOwnProperty('__forward_ref__') here ?

It does seem redundant. I actually just kept the original check here, so I'm not sure if the original author had some reason for it. Is there some reason we need to ensure that the property is not an inherited one?

I guess if there's no test for it...

maybe leave it like this for now... thanks

NickClark · 2016-09-23T23:52:45Z

Should be good to go.

vicb · 2016-09-24T00:09:00Z

Would this work ?

export function forwardRef<T>(typeFactory: () => T): ForwardRefFn<T> { typeFactory['__forward_ref__'] = forwardRef; typeFactory['toString'] = function() { return stringify(this()); }; return typeFactory; }

make __forward_ref__ a const

I'll look at this when I get back to my dev computer. Do we allow implicit any?

Also, if we use the latest TypeScript I think it's supposed to allow something like this now: forwardRefFn = <ForwardRefFn<T>>forwardRefFn

I'm changing the name to typeFactory. Not sure I'm clear on what else you're asking for here. Are we accessing object attributes via strings to help prevent minification? Seems like that wouldn't happen.

vicb · 2016-09-24T00:09:41Z

is this formatted ?

oh forget about it... that's ok with this clang-format version

Ya, the tooling didn't catch it, so I assumed we were good here. 😄

vicb · 2016-09-24T00:11:02Z

one minor nit

vicb · 2016-09-24T15:33:22Z

/cc @mhevery how well do this play with minification ? (ie 'forward_ref') is hardcoded.

Should it be return isFunction(t) && t.__forward_ref__ as ForwardRef === forwardRef;

Please remove t.hasOwnProperty('__forward_ref__') (which would not play well with minification) & rebase

vicb · 2016-10-10T23:59:29Z

could you please rebase and integrate required changes ?

NickClark · 2016-10-11T00:25:36Z

@vicb Ya, sorry about the delay, was gone on vacation.

vicb · 2016-10-11T01:18:43Z

No pb Nick, only have time to get back to this PR now

NickClark · 2016-10-12T16:05:52Z

Had some questions in regard to the requested refactor of forwardRef

vicb · 2016-10-13T00:57:11Z

Yep, what ?

NickClark · 2016-10-14T18:03:01Z

@vicb ☝️ #11813 (comment)

mhevery · 2016-10-20T18:12:33Z

@vicb ping

NickClark · 2016-10-24T19:35:27Z

@vicb Hopefully that'll do 😃

tbosch · 2017-06-06T16:12:41Z

Hi,
as this is a breaking change we are willing to take in the 5.0 release. Could you rebase against master if you are still interested in landing this PR?

NickClark · 2017-06-06T21:49:43Z

Ya, I'll try to get to this in the next day or so.

Makes `forwardRef` and `resolveForwardRef` return more meaningful types. Now when resolving a forward reference, TypeScript knows what type the returned reference is. Closes angular#11638

NickClark · 2017-06-09T01:49:53Z

I believe it's good to go.

NickClark · 2017-06-19T15:34:48Z

@tbosch Is anything else needed?

tbosch · 2017-06-27T12:55:22Z

We need to create a branch for v5 before we can land this...

splincode · 2018-02-28T23:16:40Z

@NickClark @mhevery @vicb What is the status of the task?

alexeagle · 2018-09-18T14:44:43Z

Closing pull requests from 2016, sorry this got stale and was never merged. Please open a new one if you still care about this change.

angular-automatic-lock-bot · 2019-09-14T01:51:15Z

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.}

googlebot added the cla: yes label Sep 21, 2016

NickClark force-pushed the forward-ref-11638 branch 2 times, most recently from e8b5367 to e8048d5 Compare September 22, 2016 00:36

vicb reviewed Sep 22, 2016

View reviewed changes

vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 22, 2016

NickClark force-pushed the forward-ref-11638 branch from e8048d5 to 93a47c0 Compare September 22, 2016 07:18

vicb reviewed Sep 22, 2016

View reviewed changes

NickClark force-pushed the forward-ref-11638 branch from 93a47c0 to 2a06556 Compare September 22, 2016 17:21

vicb reviewed Sep 22, 2016

View reviewed changes

vicb added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 22, 2016

NickClark force-pushed the forward-ref-11638 branch from 2a06556 to adb974b Compare September 23, 2016 01:46

vicb reviewed Sep 24, 2016

View reviewed changes

vsavkin added the area: core Issues related to the framework runtime label Oct 4, 2016

NickClark force-pushed the forward-ref-11638 branch 3 times, most recently from 82f1839 to 77ec25d Compare October 24, 2016 19:00

vicb added the breaking changes label Oct 27, 2016

chuckjaz added the type: bug/fix label May 25, 2017

tbosch removed deprecated label: LGTM (DO NOT USE ANY MORE) action: review The PR is still awaiting reviews from at least one requested reviewer action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 6, 2017

tbosch approved these changes Jun 6, 2017

View reviewed changes

tbosch added the state: Follow Up label Jun 6, 2017

NickClark force-pushed the forward-ref-11638 branch from 77ec25d to ee12e03 Compare June 8, 2017 22:28

fix(compiler): properly type forwardRef and resolveForwardRef

64b2609

Makes `forwardRef` and `resolveForwardRef` return more meaningful types. Now when resolving a forward reference, TypeScript knows what type the returned reference is. Closes angular#11638

NickClark force-pushed the forward-ref-11638 branch from ee12e03 to 64b2609 Compare June 9, 2017 01:15

mhevery force-pushed the master branch from 08b36cb to 5d4b36f Compare July 20, 2017 21:48

vicb added this to the v6-candidates milestone Feb 13, 2018

vicb added effort2: days risk: medium labels Feb 13, 2018

trotyl mentioned this pull request Jul 21, 2018

fix(core): refine Injector.get declaration to infer correct return type #24151

Closed

2 of 3 tasks

alexeagle removed this from the v6-candidates milestone Aug 10, 2018

alexeagle closed this Sep 18, 2018

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

NickClark commented Sep 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickClark commented Sep 23, 2016

Uh oh!

vicb Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Sep 24, 2016

Uh oh!

vicb Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Oct 10, 2016

Uh oh!

NickClark commented Oct 11, 2016

Uh oh!

vicb commented Oct 11, 2016

Uh oh!

NickClark commented Oct 12, 2016

Uh oh!

vicb commented Oct 13, 2016

Uh oh!

NickClark commented Oct 14, 2016

Uh oh!

mhevery commented Oct 20, 2016

Uh oh!

NickClark commented Oct 24, 2016

Uh oh!

tbosch commented Jun 6, 2017

Uh oh!

NickClark commented Jun 6, 2017

Uh oh!

NickClark commented Jun 9, 2017

vicb Sep 24, 2016 •

edited

Loading

vicb Sep 24, 2016 •

edited

Loading

splincode commented Feb 28, 2018 •

edited

Loading