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): various IE issues #34305

Closed
wants to merge 8 commits into from
Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Dec 8, 2019

Since we haven't been running the Ivy tests against IE for a while, we've accumulated some errors and test failures. These changes fix the issues which come from a handful of root causes:

i18n instructions thrown off by sanitizer:

While sanitizing on browsers that don't support the template element (pretty much only IE), we create an inert document and we insert content into it via document.body.innerHTML = unsafeHTML. The problem is that IE appears to parse the HTML passed to innerHTML differently, depending on whether the element has been inserted into a document or not. In particular, it seems to split some strings into multiple text nodes, which would've otherwise been a single node. This ended up throwing off some of the i18n code down the line and causing a handful of failures. I've worked around it by creating a new inert body element into which the HTML would be inserted.

Inheriting injectable definition from parent class not working in IE10 JIT mode:

The way definitions are added in JIT mode is through Object.defineProperty, but the problem is that in IE10 properties defined through defineProperty won't be inherited which means that inheriting injectable definitions no longer works. These changes add a workaround only for JIT mode where we define a fallback method for retrieving the definition. This isn't ideal, but it should only be required until v10 where we'll no longer support inheriting injectable definitions from undecorated classes.

Inheritance issues in IE10:

It looks like most of the inheritance functionality in Ivy wasn't working in JIT mode because we weren't accessing things correctly. These changes fix all of the cases.

Proxies being used in DebugElement.classes:

We're currently using proxies to pick up changes to element classes coming outside of Angular. Proxies aren't supported in IE and an error is always thrown. I've reworked the tests where possible and skipped the rest with a TODO to come back and re-enable them once we have some kind of fallback.

Inconsistent typeof Node value:

We have a couple of cases where we use something like typeof Node === 'function' to figure out whether we're in a worker context. This works in most browsers, but IE returns object instead of function. I've updated all the usages to account for it.

IE saving the attribute case:

In DebugElement.attributes we return all of the attributes from the underlying DOM node. Most browsers change the attribute names to lower case, but IE preserves the case and since we use camel-cased attributes, the return value was inconsistent. I've changed it to always lower case the attribute names.

Unable to get own function name:

When we log DI errors we get the name of the provider via SomeClass.name. In IE functions that inherit from other functions don't have their own name, but they take the name from the lowest parent in the chain, before Function. I've added some changes to fall back to parsing out the function name from the function's string form.

Different attribute order in innerHTML:

We've got some tests that assert that the generated DOM looks correct. The problem is that IE changes the attribute order in innerHTML which caused the tests to fail. I've reworked the relevant tests not to assert directly against innerHTML.

proto not supported:

IE doesn't support __proto__. I've added a fallback based on #34279.

.circleci/config.yml Outdated Show resolved Hide resolved
@crisbeto crisbeto force-pushed the ivy-ie-failures branch 4 times, most recently from f9bbc9b to 19755e0 Compare December 8, 2019 22:25
@crisbeto crisbeto added comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer type: bug/fix labels Dec 8, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 8, 2019
@crisbeto crisbeto marked this pull request as ready for review December 8, 2019 22:42
@crisbeto crisbeto requested review from a team as code owners December 8, 2019 22:42
@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Dec 9, 2019
@gkalpak gkalpak mentioned this pull request Dec 9, 2019
packages/core/src/render3/jit/directive.ts Outdated Show resolved Hide resolved
packages/core/src/render3/jit/directive.ts Outdated Show resolved Hide resolved
packages/core/src/di/jit/util.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Outdated Show resolved Hide resolved
@crisbeto
Copy link
Member Author

Addressed the feedback from @pkozlowski-opensource and @gkalpak.

packages/core/src/di/interface/defs.ts Outdated Show resolved Hide resolved
packages/core/src/render3/jit/directive.ts Outdated Show resolved Hide resolved
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

@crisbeto could you please pick up Paul's commits from #34277 as discussed?

Also, ideally I would like to see work-around related to Proxy usage (FW-1767) in a separate commit. The reason for this is that if we decide to merge #34328 (and it looks like we will) we might not want to merge your PR with work-arounds.

Generally speaking, I'm expecting that we are only going to merge parts of the work here so having small, focused commits would make the whole operation easier (I know it is more work for you, but it will help the overall effort)

@crisbeto
Copy link
Member Author

@pkozlowski-opensource the problem with cherry picking the commits is that the CLA bot will start complaining if there are mixed authors. I took Paul's changes just to illustrate that everything is passing, but I'll remove them before setting it as ready to merge.

Regarding the Proxy error: this is the reason why I left the TODO in there and mentioned it in the issue. If we get it fixed before this PR goes in, I'll rebase and remove my workarounds.

@crisbeto
Copy link
Member Author

The build changes have been reverted.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Thanks for all these fixes! Changes LGTM, but we should split the fixes into separate commits as @pkozlowski-opensource suggests. If there are unintended consequences (e.g. in G3) and we have to revert something, would rather not revert all changes. Maybe a natural way to split would be by section in the PR description?

@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 11, 2019
While sanitizing on browsers that don't support the `template` element (pretty much only IE), we create an inert document and we insert content into it via `document.body.innerHTML = unsafeHTML`. The problem is that IE appears to parse the HTML passed to `innerHTML` differently, depending on whether the element has been inserted into a document or not. In particular, it seems to split some strings into multiple text nodes, which would've otherwise been a single node. This ended up throwing off some of the i18n code down the line and causing a handful of failures. I've worked around it by creating a new inert `body` element into which the HTML would be inserted.
In `DebugElement.attributes` we return all of the attributes from the underlying DOM node. Most browsers change the attribute names to lower case, but IE preserves the case and since we use camel-cased attributes, the return value was inconsitent. I've changed it to always lower case the attribute names.
@kara kara added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 13, 2019
@kara
Copy link
Contributor

kara commented Dec 13, 2019

presubmit

@kara kara 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 labels Dec 13, 2019
@kara kara closed this in 0100a39 Dec 13, 2019
kara pushed a commit that referenced this pull request Dec 13, 2019
…#34305)

In `DebugElement.attributes` we return all of the attributes from the underlying DOM node. Most browsers change the attribute names to lower case, but IE preserves the case and since we use camel-cased attributes, the return value was inconsitent. I've changed it to always lower case the attribute names.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…n IE (#34305)

We have a couple of cases where we use something like `typeof Node === 'function'` to figure out whether we're in a worker context. This works in most browsers, but IE returns `object` instead of `function`. I've updated all the usages to account for it.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
We've got some tests that assert that the generate DOM looks correct. The problem is that IE changes the attribute order in `innerHTML` which caused the tests to fail. I've reworked the relevant tests not to assert directly against `innerHTML`.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…4305)

In JIT mode we use `__proto__` when reading constructor parameter metadata, however it's not supported on IE10. These changes switch to using `Object.getPrototypeOf` instead.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
Fixes the metadata and lifecycle hook inheritance not working properly in IE10, because we weren't accessing things correctly.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
… working on IE10 in JIT mode (#34305)

The way definitions are added in JIT mode is through `Object.defineProperty`, but the problem is that in IE10 properties defined through `defineProperty` won't be inherited which means that inheriting injectable definitions no longer works. These changes add a workaround only for JIT mode where we define a fallback method for retrieving the definition. This isn't ideal, but it should only be required until v10 where we'll no longer support inheriting injectable definitions from undecorated classes.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…34305)

When we log DI errors we get the name of the provider via `SomeClass.name`. In IE functions that inherit from other functions don't have their own `name`, but they take the `name` from the lowest parent in the chain, before `Function`. I've added some changes to fall back to parsing out the function name from the function's string form.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
While sanitizing on browsers that don't support the `template` element (pretty much only IE), we create an inert document and we insert content into it via `document.body.innerHTML = unsafeHTML`. The problem is that IE appears to parse the HTML passed to `innerHTML` differently, depending on whether the element has been inserted into a document or not. In particular, it seems to split some strings into multiple text nodes, which would've otherwise been a single node. This ended up throwing off some of the i18n code down the line and causing a handful of failures. I've worked around it by creating a new inert `body` element into which the HTML would be inserted.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…#34305)

In `DebugElement.attributes` we return all of the attributes from the underlying DOM node. Most browsers change the attribute names to lower case, but IE preserves the case and since we use camel-cased attributes, the return value was inconsitent. I've changed it to always lower case the attribute names.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…n IE (#34305)

We have a couple of cases where we use something like `typeof Node === 'function'` to figure out whether we're in a worker context. This works in most browsers, but IE returns `object` instead of `function`. I've updated all the usages to account for it.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
We've got some tests that assert that the generate DOM looks correct. The problem is that IE changes the attribute order in `innerHTML` which caused the tests to fail. I've reworked the relevant tests not to assert directly against `innerHTML`.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…4305)

In JIT mode we use `__proto__` when reading constructor parameter metadata, however it's not supported on IE10. These changes switch to using `Object.getPrototypeOf` instead.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
Fixes the metadata and lifecycle hook inheritance not working properly in IE10, because we weren't accessing things correctly.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
… working on IE10 in JIT mode (#34305)

The way definitions are added in JIT mode is through `Object.defineProperty`, but the problem is that in IE10 properties defined through `defineProperty` won't be inherited which means that inheriting injectable definitions no longer works. These changes add a workaround only for JIT mode where we define a fallback method for retrieving the definition. This isn't ideal, but it should only be required until v10 where we'll no longer support inheriting injectable definitions from undecorated classes.

PR Close #34305
kara pushed a commit that referenced this pull request Dec 13, 2019
…34305)

When we log DI errors we get the name of the provider via `SomeClass.name`. In IE functions that inherit from other functions don't have their own `name`, but they take the `name` from the lowest parent in the chain, before `Function`. I've added some changes to fall back to parsing out the function name from the function's string form.

PR Close #34305
@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 Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants