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.

Sign up for GitHub

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

Jump to bottom

fix(ivy): set LOCALE_ID when using the injector #31566

Closed
wants to merge 2 commits into from
Closed

fix(ivy): set LOCALE_ID when using the injector #31566

wants to merge 2 commits into from

Conversation

ocombe
Copy link
Contributor

@ocombe ocombe commented Jul 15, 2019 •
edited

In BrowserModule the value of LOCALE_ID is defined in the APPLICATION_MODULE_PROVIDERS after APP_INITIALIZER has run.
This PR ensures that LOCALE_ID is also set for ivy at the same moment which allows the application to fetch the locale from a backend (for example).

Fixes #31465

I also took the opportunity to define the default locale id as a constant for all packages (not just core/render3).

@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from 4628a05 to d33614c Compare July 15, 2019 13:57
@ocombe ocombe added area: i18n comp: ivy target: major This PR is targeted for the next major release labels Jul 15, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 15, 2019
@ocombe ocombe added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 15, 2019
@ocombe ocombe marked this pull request as ready for review July 15, 2019 14:21
@ocombe ocombe requested review from a team as code owners July 15, 2019 14:21
@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from d33614c to 223c2a2 Compare July 15, 2019 15:49
@AndrewKushnir
Copy link
Contributor

This PR will require a rebase once PR #31519 lands -> adding "blocked" label.

kara
kara approved these changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@kara kara left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, aside from tests

packages/core/test/application_module_spec.ts Outdated
import {describe, expect, inject, it} from '../testing/src/testing_internal';

{
describe('Application module', () => {
it('should set the default locale to "en-US"',
inject([LOCALE_ID], (defaultLocale: string) => { expect(defaultLocale).toEqual('en-US'); }));
it('should set the default locale to DEFAULT_LOCALE_ID',
Copy link
Contributor

@kara kara Jul 15, 2019 •
edited

Choose a reason for hiding this comment

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

I'm not sure we should change this test. It's specifically testing that the default is en-us. If someone accidentally added an f or something to DEFAULT_LOCALE_ID, this test would still pass. We should enforce having to change this test if we want to change the default (which is unlikely).

Also: It doesn't look like we added a new test to make sure we don't regress on this bug (#31465). Can you add one?

@kara kara 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 Jul 15, 2019
@kara kara added the action: presubmit The PR is in need of a google3 presubmit label Jul 15, 2019
AndrewKushnir
AndrewKushnir approved these changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, with +1 to Kara's comments.

@kara
Copy link
Contributor

kara commented Jul 15, 2019

VE presubmit

Ivy presubmit

@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from 223c2a2 to 65dbfa0 Compare July 15, 2019 19:38
@AndrewKushnir
Copy link
Contributor

@ocombe PR #31519 is now merged, could you please rebase your changes? Thank you.

@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from c9aeebc to b048640 Compare July 17, 2019 14:12
@AndrewKushnir AndrewKushnir removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Jul 17, 2019
@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from b048640 to 84e7038 Compare July 22, 2019 13:39
@ocombe
Copy link
Contributor Author

ocombe commented Jul 22, 2019

I disabled the test for ivy for now so that we can need to fix the error in view engine first.
Once the bug in r3 testbed compiler is fixed, this test should be enabled for ivy as well.

@ocombe ocombe added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 22, 2019
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 24, 2019 •
edited

@ocombe as we discussed offline, I was unable to repro the problem with VE/Ivy on the current master without other changes from this PR (the test fails for both Ivy and VE). I'm wondering if it'd be sufficient for the purposes of this PR to simplify the test that you've created to something like this:

      it('should wait for APP_INITIALIZER to set providers for `LOCALE_ID`',
          async() => {
            let locale: string = '';

            const promise = Promise.resolve().then(() => { locale = 'fr-FR'; });

            const testModule = createModule({
              providers: [
                {provide: APP_INITIALIZER, useValue: () => promise, multi: true},
                {provide: LOCALE_ID, useFactory: () => locale}
              ]
            });
            const app = await defaultPlatform.bootstrapModule(testModule);
            expect(app.injector.get(LOCALE_ID)).toEqual('fr-FR');
          });

@ocombe
fix(ivy): set LOCALE_ID when using the injector
5c97ba3 
In `BrowserModule` the value of `LOCALE_ID` is defined in the `APPLICATION_MODULE_PROVIDERS` after `APP_INITIALIZER` has run.
This PR ensures that `LOCALE_ID` is also set for ivy at the same moment which allows the application to fetch the locale from a backend (for example).

Fixes angular#31465

FW-1436 #resolve
@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from 84e7038 to cb86add Compare July 25, 2019 18:27
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jul 25, 2019
@ocombe ocombe force-pushed the fix/ivy/fw-1436-locale-id branch from cb86add to 57430b5 Compare July 25, 2019 19:36
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 25, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@ocombe @sabeersulaiman
fix(ivy): set LOCALE_ID when using the injector (angular#31566)
d5e9893 
In `BrowserModule` the value of `LOCALE_ID` is defined in the `APPLICATION_MODULE_PROVIDERS` after `APP_INITIALIZER` has run.
This PR ensures that `LOCALE_ID` is also set for ivy at the same moment which allows the application to fetch the locale from a backend (for example).

Fixes angular#31465

FW-1436 #resolve

PR Close angular#31566
@angular-automatic-lock-bot
Copy link

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 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@kara kara kara approved these changes

Assignees

@ocombe ocombe

Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

LOCALE_ID is being set before APP_INITIALIZER runs
4 participants
@ocombe @AndrewKushnir @kara @googlebot