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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n ICU expressions in Universal/App-Shell - ERROR ReferenceError: document is not defined #33651

Closed
alan-agius4 opened this issue Nov 7, 2019 · 4 comments

Comments

@alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Nov 7, 2019

馃悶 bug report

Affected Package

@angular/core

Is this a regression?

Yes, works with VE.

Description

In the CLI @clydin enabled some further tests in the CLI and i18n. Server tests are now failing when using anything which is dependent on ICU expressions such as plural. The culprit seems to be that Ivy i18n relies on the document global rather than the standard way to get the document via the DI token. This will break in universal and app-shell because document as a global doesn't exist, and it will fail with ERROR ReferenceError: document is not defined

馃敩 Minimal Reproduction

Use i18n ICU expressions in universal or app-shell scenario.

馃敟 Exception or Error

ERROR ReferenceError: document is not defined

馃實 Your Environment

Angular Version:
9.0.0-rc.0

Anything else relevant?
Exception is thrown from:

const inertBodyHelper = new InertBodyHelper(document);

Posted also in #fw-i18n

@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Nov 7, 2019

@mhevery 's suggestion:

  1. Make InertBodyHelper constructor optional for document
  2. In InertBodyHelper if document is not passed it it should try to look for it by probing the global space.
  3. First look for createHTMLDocument and if found call it
  4. Second look for document and use it to build innert document like now.
  5. Lastly it should throw an error saying that it looked for those two places did not find anything and is giving up. It should than point to some instructions on what it expects to be there.
@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Nov 7, 2019

An alternative idea ...

Move getDOM() and setDOM() to @angular/core (they are currently in @angular/common).
Then we can do getDOM().createHtmlDocument(); in the sanitiser.
And it is up to the current platform to call setDOM() with the appropriate DomAdapter.

@petebacondarwin petebacondarwin self-assigned this Nov 7, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Nov 7, 2019

New idea discussed offline with @mhevery :

import {setDocumentFactory} from '@angular/core';
setDocumentFactory(() => ...);

etc

@manughub manughub modified the milestones: needsTriage, v9-blockers Nov 7, 2019
@kara kara added the type: bug/fix label Nov 7, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Nov 9, 2019

Final idea: define setDocument() and getDocument() method for ivy.

  • Use getDocument() wherever ivy needs a global document object.
  • Use setDocument() in each of the platform packages to set the appropriate document value.

E.g.

  const inertBodyHelper = new InertBodyHelper(getDocument());

and

function _document(injector: Injector) {
  let config: PlatformConfig|null = injector.get(INITIAL_CONFIG, null);
  const document = config && config.document ? parseDocument(config.document, config.url) :
                                               getDOM().createHtmlDocument();
  // Tell ivy about the global documentsetDocument(document);
  return document;
}
petebacondarwin added a commit to petebacondarwin/angular that referenced this issue 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 DIand 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 angular#33651
petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Nov 10, 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 DIand 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 angular#33651
petebacondarwin added a commit to petebacondarwin/angular that referenced this issue 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 angular#33651
@kara kara closed this in e511bfc Nov 11, 2019
kara added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.