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(ivy): platform agnostic document #33712

Closed

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 9, 2019

Most of the use of document in the framework is within
the DI so they just inject the DOCUMENT token and are done.

Ivy is special because it does not rely upon the DI and must
get hold of the document some other way. There are a limited
number of places relevant to ivy that currently consume a global
document object.

The solution is modelled on the LOCALE_ID approach, which has
getLocaleId() and setLocaleId() top-level functions for ivy (see
core/src/render3/i18n.ts). In the rest of Angular (i.e. using DI) the
LOCALE_ID token has a provider that also calls setLocaleId() to
ensure that ivy has the same value.

This commit defines getDocument() and setDocument() top-level
functions for ivy. Wherever ivy needs the global document, it calls
getDocument() instead. Each of the platforms (e.g. Browser, Server,
WebWorker) have providers for DOCUMENT. In each of those providers
they also call setDocument() accordingly.

Fixes #33651 and #32514

@petebacondarwin petebacondarwin requested review from angular/dev-infra-framework as code owners Nov 9, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 9, 2019
@googlebot googlebot added the cla: yes label Nov 9, 2019
Copy link
Contributor

AndrewKushnir left a comment

LGTM (just one comment) 👍Thanks Pete!

packages/core/src/render3/interfaces/renderer.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir requested a review from mhevery Nov 9, 2019
@petebacondarwin petebacondarwin requested review from angular/fw-integration as code owners Nov 9, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-global-document branch from d708a82 to 8f8bc26 Nov 10, 2019
@petebacondarwin petebacondarwin removed request for angular/fw-integration and mhevery Nov 11, 2019
Copy link
Member

IgorMinar left a comment

lgtm. thanks!

It looks like there was a typo when it originally was
written. As it works right now, the `beforeEach` and
`afterEach` cancel each other out. But then
`ensureDocument()` is called anyway in the `withBody()`
function.

With this change there is no need to call `ensureDocument() in the
`withBody() function.
Most of the use of `document` in the framework is within
the DI so they just inject the `DOCUMENT` token and are done.

Ivy is special because it does not rely upon the DI and must
get hold of the document some other way. There are a limited
number of places relevant to ivy that currently consume a global
document object.

The solution is modelled on the `LOCALE_ID` approach, which has
`getLocaleId()` and `setLocaleId()` top-level functions for ivy (see
`core/src/render3/i18n.ts`).  In the rest of Angular (i.e. using DI) the
`LOCALE_ID` token has a provider that also calls setLocaleId() to
ensure that ivy has the same value.

This commit defines `getDocument()` and `setDocument() `top-level
functions for ivy. Wherever ivy needs the global `document`, it calls
`getDocument()` instead.  Each of the platforms (e.g. Browser, Server,
WebWorker) have providers for `DOCUMENT`. In each of those providers
they also call `setDocument()` accordingly.

Fixes #33651
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-global-document branch from 8f8bc26 to 384de35 Nov 11, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 11, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 11, 2019

FYI, VE and Ivy presubmits are successful.

@kara kara closed this in f747587 Nov 11, 2019
kara added a commit that referenced this pull request Nov 11, 2019
kara added a commit that referenced this pull request Nov 11, 2019
Most of the use of `document` in the framework is within
the DI so they just inject the `DOCUMENT` token and are done.

Ivy is special because it does not rely upon the DI and must
get hold of the document some other way. There are a limited
number of places relevant to ivy that currently consume a global
document object.

The solution is modelled on the `LOCALE_ID` approach, which has
`getLocaleId()` and `setLocaleId()` top-level functions for ivy (see
`core/src/render3/i18n.ts`).  In the rest of Angular (i.e. using DI) the
`LOCALE_ID` token has a provider that also calls setLocaleId() to
ensure that ivy has the same value.

This commit defines `getDocument()` and `setDocument() `top-level
functions for ivy. Wherever ivy needs the global `document`, it calls
`getDocument()` instead.  Each of the platforms (e.g. Browser, Server,
WebWorker) have providers for `DOCUMENT`. In each of those providers
they also call `setDocument()` accordingly.

Fixes #33651

PR Close #33712
kara added a commit that referenced this pull request Nov 11, 2019
It looks like there was a typo when it originally was
written. As it works right now, the `beforeEach` and
`afterEach` cancel each other out. But then
`ensureDocument()` is called anyway in the `withBody()`
function.

With this change there is no need to call `ensureDocument() in the
`withBody() function.

PR Close #33712
kara added a commit that referenced this pull request Nov 11, 2019
kara added a commit that referenced this pull request Nov 11, 2019
Most of the use of `document` in the framework is within
the DI so they just inject the `DOCUMENT` token and are done.

Ivy is special because it does not rely upon the DI and must
get hold of the document some other way. There are a limited
number of places relevant to ivy that currently consume a global
document object.

The solution is modelled on the `LOCALE_ID` approach, which has
`getLocaleId()` and `setLocaleId()` top-level functions for ivy (see
`core/src/render3/i18n.ts`).  In the rest of Angular (i.e. using DI) the
`LOCALE_ID` token has a provider that also calls setLocaleId() to
ensure that ivy has the same value.

This commit defines `getDocument()` and `setDocument() `top-level
functions for ivy. Wherever ivy needs the global `document`, it calls
`getDocument()` instead.  Each of the platforms (e.g. Browser, Server,
WebWorker) have providers for `DOCUMENT`. In each of those providers
they also call `setDocument()` accordingly.

Fixes #33651

PR Close #33712
@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-global-document branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.