Skip to content
This repository has been archived by the owner on Dec 6, 2017. It is now read-only.

fix(reflector): do not attempt to inject named params #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(reflector): do not attempt to inject named params #153

wants to merge 1 commit into from

Conversation

antingshen
Copy link
Contributor

#142 re-opened because it fails in JS due to dart2js bug: https://code.google.com/p/dart/issues/detail?id=19635

Copy of #150

Review on Reviewable

@vicb
Copy link
Contributor

vicb commented Jul 31, 2014

@antingshen I think trying to inject named parameters is the source for this angular bug (and PR).

Any ETA for this change ?

I don't really understand the description for this PR not how it does relates the the linked dartbug. Could you please provide some more info ? Thanks

Edit:

May be this PR is a temporary workaround until the bug is fixed and then it will be reverted to allow this when supported in JS ?

If this is the case, it might be great to add some info in the changelog.

@antingshen
Copy link
Contributor Author

This PR would allow DI to inject classes that have named params by ignoring them altogether.

However, the Dart bug makes it so that mirrored constructors with named params cannot be instantiated at all, even if you don't try to pass in a value for the named param. (aka this PR should not be merged right now)

dart2js team says they'll start working on this bug second week of August.

@vicb
Copy link
Contributor

vicb commented Sep 8, 2014

@antingshen what is the status of this PR, ready to be merged ?

@antingshen
Copy link
Contributor Author

The dart bug seems to still be open, but the tests are passing, so I'm not sure. Perhaps try testing on an older version of Dart since pubspec still says >= 1.0.0.

@rkirov rkirov assigned antingshen and pavelgj and unassigned antingshen Jan 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants