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): TestBed.get(Compiler) throws "Error: Runtime compiler is not loaded" #27223

Closed
wants to merge 3 commits into from
Closed

fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is not loaded" #27223

wants to merge 3 commits into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Nov 22, 2018 •
edited

This PR adds support for the TestBed.get(Compiler) and unblocks FW-561.

fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is not loaded"

BREAKING CHANGE:

The public API for DebugNode was accidentally too broad. This change removes

  1. Public constructor. Since DebugNode is a way for Angular to communicate information
    on to the developer there is no reason why the developer should ever need to
    Instantiate the DebugNode
  2. We are also removing removeChild, addChild, insertBefore, and insertChildAfter.
    All of these methods are used by Angular to constructor the correct DebugNode tree.
    There is no reason why the developer should ever be constructing a DebugNode tree
    And these methods should have never been made public.
  3. All properties have been change to readonly since DebugNode is used by Angular
    to communicate to developer and there is no reason why these APIs should be writable.

While technically breaking change we don’t expect anyone to be effected by this change.

@mary-poppins
Copy link

You can preview e1b3860 at https://pr27223-e1b3860.ngbuilds.io/.

pkozlowski-opensource
pkozlowski-opensource reviewed Nov 22, 2018
View reviewed changes
packages/core/src/debug/debug_node.ts
export class DebugNode {
listeners: EventListener[] = [];
parent: DebugElement|null = null;
export interface DebugNode {
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, technically speaking, right? I mean, previously people could import and instantiate DebugNode and it was part of the public API.

I don't suspect that many people actually do it but we should still document this as a breaking change

Copy link
Contributor Author

@mhevery mhevery Nov 23, 2018 •
edited

Choose a reason for hiding this comment

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

@IgorMinar

From my point of view we have accidentally exposed more API than we intended to.

  • We exposed the constructor
  • We exposed the mutation methods

By changing this from class to interface I think we are only removing the API which was accidentally exposed and which serves no purpose to the end user.

PS: I will better document this.

Copy link
Member

Choose a reason for hiding this comment

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

@mhevery Could you add to the list of breaking changes that instanceof DebugNode checks can no longer be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoostK will add.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mhevery it should be fine to change this behavior, but we need to document it.

pkozlowski-opensource
pkozlowski-opensource requested changes Nov 22, 2018
View reviewed changes
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.

@mhevery I've left few comments in the code, the most important being about breaking changes. I was trying to take the same approach in the past but I got scared off the breaking changes with triggering event handlers etc. Basically we would loose the abstraction over native platform and this sounds like "big deal". Let's discuss.

If we decide to go ahead with this approach it is not clear for me what should happen with https://github.com/angular/angular/blob/master/packages/core/src/render3/debug.ts and all infrastructure that plugs it (https://github.com/angular/angular/blob/master/packages/core/testing/src/r3_test_bed.ts#L426-L429)

Lastly, the CI is red with some tests failing.

@mary-poppins
Copy link

You can preview afe9b25 at https://pr27223-afe9b25.ngbuilds.io/.

@mhevery
Copy link
Contributor Author

mhevery commented Nov 23, 2018

If we decide to go ahead with this approach it is not clear for me what should happen with https://github.com/angular/angular/blob/master/packages/core/src/render3/debug.ts and all infrastructure that plugs it (https://github.com/angular/angular/blob/master/packages/core/testing/src/r3_test_bed.ts#L426-L429)

Ahh that is an excellent Catch! Yes that code is dead and I have now deleted it. :-)

@mary-poppins
Copy link

You can preview bb8f9cb at https://pr27223-bb8f9cb.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource added target: major This PR is targeted for the next major release comp: ivy labels Nov 23, 2018
@ngbot ngbot bot added this to the needsTriage milestone Nov 23, 2018
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 23, 2018
pkozlowski-opensource
pkozlowski-opensource approved these changes Nov 23, 2018
View reviewed changes
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.

@mhevery if you are positive that we can implement the rest of the DebugElement methods this LGTM
Would be probably good if someone else can do another pass on it as we are touching many parts

@mhevery
Copy link
Contributor Author

mhevery commented Nov 26, 2018

http://test/OCL:222839683:BASE:222839704:1543254316995:47c83647

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 26, 2018
kara
kara approved these changes Nov 26, 2018
View reviewed changes
packages/common/test/directives/ng_component_outlet_spec.ts Outdated Show resolved Hide resolved
packages/core/src/render3/assert.ts Outdated Show resolved Hide resolved
packages/core/test/linker/ng_module_integration_spec.ts Outdated Show resolved Hide resolved
@mhevery mhevery force-pushed the testbed_core branch 2 times, most recently from c479d06 to dfbf641 Compare November 27, 2018 01:46
@mary-poppins
Copy link

You can preview c479d06 at https://pr27223-c479d06.ngbuilds.io/.

@mary-poppins
Copy link

You can preview dfbf641 at https://pr27223-dfbf641.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 2564f80 at https://pr27223-2564f80.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 07c95ec at https://pr27223-07c95ec.ngbuilds.io/.

@mhevery
Copy link
Contributor Author

mhevery commented Nov 27, 2018

http://test/OCL:222839683:BASE:222930629:1543292757503:68277596

IgorMinar
IgorMinar approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

The rest looks good

packages/core/src/metadata/directives.ts Show resolved Hide resolved
tools/public_api_guard/core/testing.d.ts Show resolved Hide resolved
@mhevery mhevery mentioned this pull request Nov 27, 2018
Closed
@mary-poppins
Copy link

You can preview 8ecc627 at https://pr27223-8ecc627.ngbuilds.io/.

@mhevery
Copy link
Contributor Author

mhevery commented Nov 27, 2018

http://test/OCL:222881368:BASE:222938072:1543299184520:907394cc

@mary-poppins
Copy link

You can preview 9028161 at https://pr27223-9028161.ngbuilds.io/.

@pkozlowski-opensource
Copy link
Member

@mhevery looks like CI is red (the golden symbols file need updating etc.)

@mhevery
Copy link
Contributor Author

mhevery commented Nov 27, 2018

http://test/OCL:222839683:BASE:223003683:1543339823046:c3644f5e

@mary-poppins
Copy link

You can preview 8ee99af at https://pr27223-8ee99af.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7e65c8a at https://pr27223-7e65c8a.ngbuilds.io/.

@mhevery
build: ts-api-guardian should support interface with value types
68b08eb 
This fixes an issue where a value would hide the type.

```
export interface Foo {
  someMethod(): void;
}

export const Foo: Function = ...;
```

In the above example the `Foo` constant will hide the `interface Foo` symbol.
This change properly saves the interface in addition to the type.
@mhevery
fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is no…
8504136 
…t loaded"

BREAKING CHANGE:

The public API for `DebugNode` was accidentally too broad. This change removes
1. Public constructor. Since `DebugNode` is a way for Angular to communicate information
   on to the developer there is no reason why the developer should ever need to
   Instantiate the `DebugNode`
2. We are also removing `removeChild`, `addChild`, `insertBefore`, and `insertChildAfter`.
   All of these methods are used by Angular to constructor the correct `DebugNode` tree.
   There is no reason why the developer should ever be constructing a `DebugNode` tree
   And these methods should have never been made public.
3. All properties have been change to `readonly` since `DebugNode` is used by Angular
   to communicate to developer and there is no reason why these APIs should be writable.

While technically breaking change we don’t expect anyone to be effected by this change.
@mary-poppins
Copy link

You can preview 8504136 at https://pr27223-8504136.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f6f9612 at https://pr27223-f6f9612.ngbuilds.io/.

@jasonaden jasonaden closed this in 60e403b Nov 27, 2018
jasonaden pushed a commit that referenced this pull request Nov 27, 2018
@mhevery @jasonaden
fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is no…
39e426c 
…t loaded" (#27223)

BREAKING CHANGE:

The public API for `DebugNode` was accidentally too broad. This change removes
1. Public constructor. Since `DebugNode` is a way for Angular to communicate information
   on to the developer there is no reason why the developer should ever need to
   Instantiate the `DebugNode`
2. We are also removing `removeChild`, `addChild`, `insertBefore`, and `insertChildAfter`.
   All of these methods are used by Angular to constructor the correct `DebugNode` tree.
   There is no reason why the developer should ever be constructing a `DebugNode` tree
   And these methods should have never been made public.
3. All properties have been change to `readonly` since `DebugNode` is used by Angular
   to communicate to developer and there is no reason why these APIs should be writable.

While technically breaking change we don’t expect anyone to be effected by this change.

PR Close #27223
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Nov 27, 2018
@petebacondarwin
test(upgrade): re-enable tests that have been fixed
4c75b18 
The fix in angular#27223 unblocks these tests
@petebacondarwin petebacondarwin mentioned this pull request Nov 27, 2018
Closed
IgorMinar pushed a commit that referenced this pull request Nov 30, 2018
@petebacondarwin @IgorMinar
test(upgrade): re-enable tests that have been fixed (#27305)
1402175 
The fix in #27223 unblocks these tests

PR Close #27305
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@mhevery @FrederikSchlemmer
build: ts-api-guardian should support interface with value types (ang…
a137fb8 
…ular#27223)

This fixes an issue where a value would hide the type.

```
export interface Foo {
  someMethod(): void;
}

export const Foo: Function = ...;
```

In the above example the `Foo` constant will hide the `interface Foo` symbol.
This change properly saves the interface in addition to the type.

PR Close angular#27223
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@mhevery @FrederikSchlemmer
fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is no…
c6d82dd 
…t loaded" (angular#27223)

BREAKING CHANGE:

The public API for `DebugNode` was accidentally too broad. This change removes
1. Public constructor. Since `DebugNode` is a way for Angular to communicate information
   on to the developer there is no reason why the developer should ever need to
   Instantiate the `DebugNode`
2. We are also removing `removeChild`, `addChild`, `insertBefore`, and `insertChildAfter`.
   All of these methods are used by Angular to constructor the correct `DebugNode` tree.
   There is no reason why the developer should ever be constructing a `DebugNode` tree
   And these methods should have never been made public.
3. All properties have been change to `readonly` since `DebugNode` is used by Angular
   to communicate to developer and there is no reason why these APIs should be writable.

While technically breaking change we don’t expect anyone to be effected by this change.

PR Close angular#27223
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@petebacondarwin @FrederikSchlemmer
test(upgrade): re-enable tests that have been fixed (angular#27305)
f0e30e4 
The fix in angular#27223 unblocks these tests

PR Close angular#27305
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@mhevery @ngfelixl
build: ts-api-guardian should support interface with value types (ang…
ed55ae9 
…ular#27223)

This fixes an issue where a value would hide the type.

```
export interface Foo {
  someMethod(): void;
}

export const Foo: Function = ...;
```

In the above example the `Foo` constant will hide the `interface Foo` symbol.
This change properly saves the interface in addition to the type.

PR Close angular#27223
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@mhevery @ngfelixl
fix(ivy): TestBed.get(Compiler) throws "Error: Runtime compiler is no…
d285f4b 
…t loaded" (angular#27223)

BREAKING CHANGE:

The public API for `DebugNode` was accidentally too broad. This change removes
1. Public constructor. Since `DebugNode` is a way for Angular to communicate information
   on to the developer there is no reason why the developer should ever need to
   Instantiate the `DebugNode`
2. We are also removing `removeChild`, `addChild`, `insertBefore`, and `insertChildAfter`.
   All of these methods are used by Angular to constructor the correct `DebugNode` tree.
   There is no reason why the developer should ever be constructing a `DebugNode` tree
   And these methods should have never been made public.
3. All properties have been change to `readonly` since `DebugNode` is used by Angular
   to communicate to developer and there is no reason why these APIs should be writable.

While technically breaking change we don’t expect anyone to be effected by this change.

PR Close angular#27223
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@petebacondarwin @ngfelixl
test(upgrade): re-enable tests that have been fixed (angular#27305)
2ceedfb 
The fix in angular#27223 unblocks these tests

PR Close angular#27305
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoostK JoostK JoostK left review comments

@IgorMinar IgorMinar IgorMinar approved these changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@kara kara kara approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker 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.

None yet
7 participants
@mhevery @mary-poppins @pkozlowski-opensource @JoostK @IgorMinar @kara @googlebot