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

Fix service configuration with isolate = true #531

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atiertant
Copy link

no more need to repeat setDefaultLang and use on every service in lazy loaded modules

@mebibou
Copy link

mebibou commented Oct 9, 2017

I think this would solve #602 , #444 and #425 so seems like an important fix, could this be reviewed and merged?

@ocombe
Copy link
Member

ocombe commented Oct 9, 2017

Hmm so what does it change actually? Could I get some tests to make sure that it fixes the problem ?

@mebibou
Copy link

mebibou commented Oct 9, 2017

@ocombe I don't know if it works, but the idea of not having to sync the children modules with the root one is clearly a good one. As said in my comment on another PR, the current forChild doesn't work unless we call use(lang) in every children, then sync lang changes between root and children. It's very hard to maintain and doesn't work out of the box

@nickwinger
Copy link

nickwinger commented Oct 23, 2017

When will this be released ?
Currently it doesn't work at all, to merge-load translations with lazy modules
(of course you can do a workaround with setTranslation, but the recommended way should of course be via the loader...)

@ocombe
Copy link
Member

ocombe commented Oct 23, 2017

Once the PR has some tests :)

@nickwinger
Copy link

so, no tests after 14 days, i guess i will stick with the workaround, or provide my own translation service...
thx

@akarabach
Copy link

Any updates here ?

@ocombe ocombe force-pushed the master branch 13 times, most recently from 8dd86e6 to 303b81f Compare November 23, 2017 15:49
@darkurse
Copy link

darkurse commented Dec 5, 2017

This fix would be really welcome

@devr24
Copy link

devr24 commented Feb 26, 2018

Guys, are you going to merge this change into the main branch? We could really use this useful change!

@Krishnakanth94
Copy link

can any one please review and merge this pull request.Thanks in Advance.

@JulianSanio
Copy link

Anybody who could resolve the conflict and approve this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.