ngUpgrade AoT #12239

Closed
wants to merge 2 commits into
from

Conversation

@petebacondarwin
Member

petebacondarwin commented Oct 12, 2016

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@@ -23,7 +23,7 @@ module.exports = function(config) {
'node_modules/core-js/client/core.js',
// include Angular v1 for upgrade module testing
- 'node_modules/angular/angular.min.js',
+ 'node_modules/angular/angular.js',

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

TODO: remember to revert this change

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

TODO: remember to revert this change

This comment has been minimized.

@alexeagle

alexeagle Oct 19, 2016

Collaborator

can revert it?

@alexeagle

alexeagle Oct 19, 2016

Collaborator

can revert it?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery said it can stay un-minified

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery said it can stay un-minified

@mhevery

Awesome start and great progress!

modules/@angular/upgrade/index.ts
@@ -12,5 +12,6 @@
* Entry point for all public APIs of the upgrade package.
*/
export * from './src/upgrade';
+export * from './src/upgrade_module';

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

Don't export all. Instead list which items specifically you want to export. Otherwise $injectorFactory and friends will be part of public API.

@mhevery

mhevery Oct 12, 2016

Member

Don't export all. Instead list which items specifically you want to export. Otherwise $injectorFactory and friends will be part of public API.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

Right, OK. I'll fix up.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

Right, OK. I'll fix up.

+@NgModule({
+ providers: [
+ { provide: '$injector', useFactory: $injectorFactory },
+ { provide: '$rootScope', useFactory: $rootScopeFactory, deps: ['$injector']},

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

@chuckjaz Notice how we have to explicitly list $rootScopeFactory and $compileFactory in future we will have many more. It would be nice to just have one function which does it all, but we get an Error:

Metadata collected contains an error that will be reported at runtime:
   Function calls are not supported.
   Consider replacing the function or lambda with a reference to an exported function

I thought we supported this, but apparently not. Could you work with @petebacondarwin and help him resolve it?

@mhevery

mhevery Oct 12, 2016

Member

@chuckjaz Notice how we have to explicitly list $rootScopeFactory and $compileFactory in future we will have many more. It would be nice to just have one function which does it all, but we get an Error:

Metadata collected contains an error that will be reported at runtime:
   Function calls are not supported.
   Consider replacing the function or lambda with a reference to an exported function

I thought we supported this, but apparently not. Could you work with @petebacondarwin and help him resolve it?

This comment has been minimized.

@chuckjaz

chuckjaz Oct 12, 2016

Member

@mhevery Sure.

@petebacondarwin Can I see the function that you are trying to use? It might be easy to transform.

@chuckjaz

chuckjaz Oct 12, 2016

Member

@mhevery Sure.

@petebacondarwin Can I see the function that you are trying to use? It might be easy to transform.

+ providers: [
+ { provide: '$injector', useFactory: $injectorFactory },
+ { provide: '$rootScope', useFactory: $rootScopeFactory, deps: ['$injector']},
+ { provide: '$compile', useFactory: $compileFactory, deps: ['$injector']}

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

Should we re-export all of the $* items?

@mhevery

mhevery Oct 12, 2016

Member

Should we re-export all of the $* items?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

There are lots of them...
Perhaps we could just provide a recipe in the docs for people to add their own to their subclass of UpgradeModule?

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

There are lots of them...
Perhaps we could just provide a recipe in the docs for people to add their own to their subclass of UpgradeModule?

+/**
+ * UpgradeModule is the base class for the module that will contain all the
+ * information about what Angular providers and component are to be bridged
+ * between Angular 1 and Angular 2+

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

We will have to update this doc with an example. We now require that full API documentation will be included at the time of submission of the PR.

//cc @IgorMinar

@mhevery

mhevery Oct 12, 2016

Member

We will have to update this doc with an example. We now require that full API documentation will be included at the time of submission of the PR.

//cc @IgorMinar

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

These docs were mostly placeholders to help with my understanding.
We can add more as the work stabilizes

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

These docs were mostly placeholders to help with my understanding.
We can add more as the work stabilizes

+ * Usage:
+ *
+ * ```
+ * angular1Module.factory('someService', ng2ProviderFactory(SomeService))

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

turn these into running examples. See: #11773 for how the docs should look / work.

@mhevery

mhevery Oct 12, 2016

Member

turn these into running examples. See: #11773 for how the docs should look / work.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

will do as things stabilize

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

will do as things stabilize

+ function directiveFactory(
+ ng1Injector: angular.IInjectorService,
+ parse: angular.IParseService): angular.IDirective {
+ const NG2_INJECTOR = 'ng2.Injector';

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

make sure we have consistent naming. either ng2.foo or ng2Foo

@mhevery

mhevery Oct 12, 2016

Member

make sure we have consistent naming. either ng2.foo or ng2Foo

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

ng2.Injector is taken directly from the rest of the ngUpgrade codebase. I don't know how much of a breaking change it would be that code if we changed it and I think that it would be nice to keep the two forms of the library consistent.

But as @robwormald points out we probably shouldn't have ng2 in this...

Any better naming ideas for this?

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

ng2.Injector is taken directly from the rest of the ngUpgrade codebase. I don't know how much of a breaking change it would be that code if we changed it and I think that it would be nice to keep the two forms of the library consistent.

But as @robwormald points out we probably shouldn't have ng2 in this...

Any better naming ideas for this?

This comment has been minimized.

@mhevery

mhevery Oct 13, 2016

Member

I see the point with ng2. So maybe we should just do $injector and ng.injector? as the two ways. Renaming it would be a breaking change, even thought it is not part of the public API so it is unlikely that anyone would be using it. But sounds like the ngUpgradeAoT should use ng.Injector

@mhevery

mhevery Oct 13, 2016

Member

I see the point with ng2. So maybe we should just do $injector and ng.injector? as the two ways. Renaming it would be a breaking change, even thought it is not part of the public API so it is unlikely that anyone would be using it. But sounds like the ngUpgradeAoT should use ng.Injector

+ this.ngZone = ng2Injector.get(NgZone);
+ }
+
+ ngDoBootstrap() {}

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

add comment as to which this method is here.

@mhevery

mhevery Oct 12, 2016

Member

add comment as to which this method is here.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

strangely I did have a comment here but it seems to have failed to make it into the commit that was pushed.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

strangely I did have a comment here but it seems to have failed to make it into the commit that was pushed.

+
+ // Only add the config if it is there
+ // QUESTION? Do we really need this param?
+ if (config) {

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

why is this necessary? Wouldn't angular.bootstrap take care of it?

@mhevery

mhevery Oct 12, 2016

Member

why is this necessary? Wouldn't angular.bootstrap take care of it?

+
+ it('should have angular 1 loaded', () => expect(angular.version.major).toBe(1));
+
+ it('should instantiate ng2 in ng1 template and project content', async(() => {

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

@juliemr we are having a hard time making the async work here. We are getting timeout. Could you work with @petebacondarwin on figuring out why it is failing to stabilize?

@mhevery

mhevery Oct 12, 2016

Member

@juliemr we are having a hard time making the async work here. We are getting timeout. Could you work with @petebacondarwin on figuring out why it is failing to stabilize?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

I think this may have resolved itself...

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

I think this may have resolved itself...

+ html('<div>{{ \'ng1[\' }}<ng2>~{{ \'ng-content\' }}~</ng2>{{ \']\' }}</div>');
+
+ platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
+ ref.instance.bootstrapNg1(element, [ng1Module.name]);

This comment has been minimized.

@mhevery

mhevery Oct 12, 2016

Member

Why do it here, rather than in the Ng2Module constructor?

@mhevery

mhevery Oct 12, 2016

Member

Why do it here, rather than in the Ng2Module constructor?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

Moving the call to bootstrapNg1 outside the constructor makes the custom UpgradeModule more reusable. It doesn't tie it into always bootstrapping on a specific element.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

Moving the call to bootstrapNg1 outside the constructor makes the custom UpgradeModule more reusable. It doesn't tie it into always bootstrapping on a specific element.

This comment has been minimized.

@mhevery

mhevery Oct 13, 2016

Member

I see, good point.

@mhevery

mhevery Oct 13, 2016

Member

I see, good point.

@mhevery mhevery changed the title from Ngupgrade aot to ngUpgrade AoT Oct 12, 2016

+ */
+export function ng1ServiceProvider({provide, ng1Token}: {provide: any, ng1Token: any}): FactoryProvider {
+ return {provide: provide, useFactory: (i: angular.IInjectorService) => i.get(ng1Token), deps: ['$injector']};
+}

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

shouldn't this helper fall foul of the same problems with not being allowed to use a non-exported named function as a factory?
cc @mhevery & @chuckjaz

@petebacondarwin

petebacondarwin Oct 12, 2016

Member

shouldn't this helper fall foul of the same problems with not being allowed to use a non-exported named function as a factory?
cc @mhevery & @chuckjaz

This comment has been minimized.

@chuckjaz

chuckjaz Oct 12, 2016

Member

Yes. The lambda here doesn't help you.

@chuckjaz

chuckjaz Oct 12, 2016

Member

Yes. The lambda here doesn't help you.

- // }
- // };
- // });
+ it('should properly run cleanup when ng1 directive is destroyed', async(() => {

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 13, 2016

Member

@gkalpak can you take a look and see if this test makes sense. The previous commented out test that this replaces is very different and in I think doesn't even work properly.

@petebacondarwin

petebacondarwin Oct 13, 2016

Member

@gkalpak can you take a look and see if this test makes sense. The previous commented out test that this replaces is very different and in I think doesn't even work properly.

This comment has been minimized.

@gkalpak

gkalpak Oct 13, 2016

Member

The test makes sense. I think the previous test tried to do the same.
One thing I noticed is that the previous test calls ref.dispose() at the end. Is it necessary? What was the purpose?

@gkalpak

gkalpak Oct 13, 2016

Member

The test makes sense. I think the previous test tried to do the same.
One thing I noticed is that the previous test calls ref.dispose() at the end. Is it necessary? What was the purpose?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

The ref.dispose() came from how the adaptor worked. I don't believe it is necessary here. We are already calling platform.dispose() in an afterEach block.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

The ref.dispose() came from how the adaptor worked. I don't believe it is necessary here. We are already calling platform.dispose() in an afterEach block.

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 13, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 13, 2016

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Oct 13, 2016

Member

@gkalpak please do the honors for the googlebot

Member

petebacondarwin commented Oct 13, 2016

@gkalpak please do the honors for the googlebot

@@ -51,7 +51,7 @@ export interface IRootScopeService {
[key: string]: any;
}
export interface IScope extends IRootScopeService {};
-export interface IAngularBootstrapConfig {}
+export type IAngularBootstrapConfig = IInjectable;

This comment has been minimized.

@gkalpak

gkalpak Oct 13, 2016

Member

But it is not the same. IAngularBootstrapConfig is the configuration that can be passed to angular.bootstrap and can contain stringDi: true/false for example. (It can also contain debugInfoEnabled, but that is undocumented and used internally.) Essentially, it is just an object with specific properties.

@gkalpak

gkalpak Oct 13, 2016

Member

But it is not the same. IAngularBootstrapConfig is the configuration that can be passed to angular.bootstrap and can contain stringDi: true/false for example. (It can also contain debugInfoEnabled, but that is undocumented and used internally.) Essentially, it is just an object with specific properties.

This comment has been minimized.

@gkalpak

gkalpak Oct 13, 2016

Member

So, it should probably be changed to:

export interface IAngularBootstrapConfig {
  strictDi?: boolean;
}
@gkalpak

gkalpak Oct 13, 2016

Member

So, it should probably be changed to:

export interface IAngularBootstrapConfig {
  strictDi?: boolean;
}
@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Oops

On 13 Oct 2016 22:04, "Georgios Kalpakas" notifications@github.com wrote:

@gkalpak commented on this pull request.

In modules/@angular/upgrade/src/angular_js.ts
#12239:

@@ -51,7 +51,7 @@ export interface IRootScopeService {
[key: string]: any;
}
export interface IScope extends IRootScopeService {};
-export interface IAngularBootstrapConfig {}
+export type IAngularBootstrapConfig = IInjectable;

So, it should probably be changed to:

export interface IAngularBootstrapConfig {
strictDi?: boolean;
}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12239, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA9J-80qsnq9YruZZabnZy49Ezy2RyVks5qzpzTgaJpZM4KUxSw
.

Member

petebacondarwin commented Oct 14, 2016

Oops

On 13 Oct 2016 22:04, "Georgios Kalpakas" notifications@github.com wrote:

@gkalpak commented on this pull request.

In modules/@angular/upgrade/src/angular_js.ts
#12239:

@@ -51,7 +51,7 @@ export interface IRootScopeService {
[key: string]: any;
}
export interface IScope extends IRootScopeService {};
-export interface IAngularBootstrapConfig {}
+export type IAngularBootstrapConfig = IInjectable;

So, it should probably be changed to:

export interface IAngularBootstrapConfig {
strictDi?: boolean;
}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12239, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA9J-80qsnq9YruZZabnZy49Ezy2RyVks5qzpzTgaJpZM4KUxSw
.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Oct 14, 2016

Member

@googlebot, I confirm that I'm okay with my commits being contributed to Google 😃

Member

gkalpak commented Oct 14, 2016

@googlebot, I confirm that I'm okay with my commits being contributed to Google 😃

@@ -342,6 +345,9 @@ export function main() {
platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
const adapter = ref.injector.get(Ng1Adapter) as Ng1Adapter;
adapter.bootstrapNg1(element, [ng1Module.name]);
+ // the fact that the body contains the correct text means that the
+ // downgraded component was able to access the moduleInjector
+ // (since there is no other injector in this system)

This comment has been minimized.

@gkalpak

gkalpak Oct 14, 2016

Member

Still not convinced 😛

@gkalpak

gkalpak Oct 14, 2016

Member

Still not convinced 😛

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Trouble is until we have upgradeComponent working we can't do better than this test

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Trouble is until we have upgradeComponent working we can't do better than this test

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Unless you can show otherwise

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Unless you can show otherwise

@mhevery

Excellent progress sir!

@@ -6,20 +6,30 @@
* found in the LICENSE file at https://angular.io/license
*/
+export type Ng1Token = string;

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

what is the value of aliasing string to Ng1Token

@mhevery

mhevery Oct 14, 2016

Member

what is the value of aliasing string to Ng1Token

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

I was looking forward to if/when we might allow non-string tokens in ng1

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

I was looking forward to if/when we might allow non-string tokens in ng1

@@ -6,20 +6,30 @@
* found in the LICENSE file at https://angular.io/license
*/
+export type Ng1Token = string;
+
+export interface IAnnotatedFunction extends Function {

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

as soon as #11773 gets in, you will no longer need local definitions like this one.

@mhevery

mhevery Oct 14, 2016

Member

as soon as #11773 gets in, you will no longer need local definitions like this one.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Well I didn't know about https://www.npmjs.com/package/@types/angular.
@gkalpak and I probably wasted a bit of time over the last few days tweaking this file.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Well I didn't know about https://www.npmjs.com/package/@types/angular.
@gkalpak and I probably wasted a bit of time over the last few days tweaking this file.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 16, 2016

Member

I think we would need to do some work with that repository as I am not entirely happy with all of the hand crafted types there.

@petebacondarwin

petebacondarwin Oct 16, 2016

Member

I think we would need to do some work with that repository as I am not entirely happy with all of the hand crafted types there.

+
+export function downgradeComponent(info: ComponentInfo) : Function {
+
+ const idPrefix = `NG2_UPGRADE_${downgradeCount++}_`;

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

drop NG2 as we said no 2 in the names.

@mhevery

mhevery Oct 14, 2016

Member

drop NG2 as we said no 2 in the names.

This comment has been minimized.

+
+ return {
+ restrict: 'E',
+ require: '?^' + UPGRADE_MODULE_INJECTOR,

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

change UPGRADE_MODULE_INJECTOR to COMPONENT_INJECTOR

@mhevery

mhevery Oct 14, 2016

Member

change UPGRADE_MODULE_INJECTOR to COMPONENT_INJECTOR

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

OK, do we still leave the UPGRADE_MODULE_INJECTOR constant for when we are accessing the top level injector? E.g. $injector.get(UPGRADE_MODULE_INJECTOR)

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

OK, do we still leave the UPGRADE_MODULE_INJECTOR constant for when we are accessing the top level injector? E.g. $injector.get(UPGRADE_MODULE_INJECTOR)

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

correct

@mhevery

mhevery Oct 14, 2016

Member

correct

+ */
+@NgModule({
+ providers: [
+ Ng1Adapter,

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

Why is this broken up to a separate class? Why no just part of the module? I am on the fence, and wanted to understand your reasoning.

@mhevery

mhevery Oct 14, 2016

Member

Why is this broken up to a separate class? Why no just part of the module? I am on the fence, and wanted to understand your reasoning.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

I gave my reasoning on the ngUpgrade Slack channel and in the commit for this 6beed8d

Basically, I personally found using the NgModule class to do the bootstrap confusing.

In my understanding NgModules are about statically defining information about the compilation and are not "active" in the sense that you call them to do stuff.

I could be convinced to rejoin them but I think with the right name for the service it makes it adheres better to the Principle of Least Surprise.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

I gave my reasoning on the ngUpgrade Slack channel and in the commit for this 6beed8d

Basically, I personally found using the NgModule class to do the bootstrap confusing.

In my understanding NgModules are about statically defining information about the compilation and are not "active" in the sense that you call them to do stuff.

I could be convinced to rejoin them but I think with the right name for the service it makes it adheres better to the Principle of Least Surprise.

@@ -7,10 +7,11 @@
*/
export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';
export const NG2_COMPONENT_FACTORY_REF_MAP = 'ng2.ComponentFactoryRefMap';

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

not neede

@mhevery

mhevery Oct 14, 2016

Member

not neede

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Except that it is still used by the JIT UpgradeAdapter

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Except that it is still used by the JIT UpgradeAdapter

@@ -7,10 +7,11 @@
*/
export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';
export const NG2_COMPONENT_FACTORY_REF_MAP = 'ng2.ComponentFactoryRefMap';
export const NG2_ZONE = 'ng2.NgZone';

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

not needed

@mhevery

mhevery Oct 14, 2016

Member

not needed

+ entryComponents: [Ng2Component]
+ })
+ class Ng2Module {
+ ngDoBootstrap() {}

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

nice thing about extending was that one did not have to write ngDoBootstrap method.

@mhevery

mhevery Oct 14, 2016

Member

nice thing about extending was that one did not have to write ngDoBootstrap method.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

That is true. But it still rankled that we were the UpgradeModule in its free standing class form as an NgModule import; and then also creating a subclass of UpgradeModule purely to inherit this one method.

We are stuck between two variants of unwanted boilerplate.
But it did occur to me that we could get Angular-CLI to generate this, so either way it shouldn't matter too much.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

That is true. But it still rankled that we were the UpgradeModule in its free standing class form as an NgModule import; and then also creating a subclass of UpgradeModule purely to inherit this one method.

We are stuck between two variants of unwanted boilerplate.
But it did occur to me that we could get Angular-CLI to generate this, so either way it shouldn't matter too much.

+ // the ng1 app module that will consume the downgraded component
+ const ng1Module = angular.module('ng1', [])
+ // create an ng1 facade of the ng2 component
+ .directive('ng2', downgradeComponent({ selector: 'ng2', type: Ng2Component }));

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

Why do we have to pass in the selector? it should not be needed.

Also can we rename type to component?

@mhevery

mhevery Oct 14, 2016

Member

Why do we have to pass in the selector? it should not be needed.

Also can we rename type to component?

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

You are right. I was being lazy because I was reusing DowngradeNg2ComponentAdapter from the JIT version which accepts a ComponentInfo type, which requires the selector and type properties.

In fact the only use of info.selector in that class is when reporting an error.

We should refactor this and remove the selector.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

You are right. I was being lazy because I was reusing DowngradeNg2ComponentAdapter from the JIT version which accepts a ComponentInfo type, which requires the selector and type properties.

In fact the only use of info.selector in that class is when reporting an error.

We should refactor this and remove the selector.

+ html('<div>{{ \'ng1[\' }}<ng2>~{{ \'ng-content\' }}~</ng2>{{ \']\' }}</div>');
+
+ platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
+ const adapter = ref.injector.get(Ng1Adapter) as Ng1Adapter;

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

By having Ng1Adapter as a separate class, there is more things to import. Still on the fence about that.

@mhevery

mhevery Oct 14, 2016

Member

By having Ng1Adapter as a separate class, there is more things to import. Still on the fence about that.

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Let's mull it over during the weekend. Happy to revert if it still feels wrong.

@petebacondarwin

petebacondarwin Oct 14, 2016

Member

Let's mull it over during the weekend. Happy to revert if it still feels wrong.

+
+ @NgModule({
+ declarations: [Ng1Component, Ng2Component],
+ imports: [BrowserModule]

This comment has been minimized.

@mhevery

mhevery Oct 14, 2016

Member

Ng1Module => UpgradeModule

@mhevery

mhevery Oct 14, 2016

Member

Ng1Module => UpgradeModule

@@ -27,8 +27,8 @@ export interface AttrProp {
export interface ComponentInfo {

This comment has been minimized.

@alexeagle

alexeagle Oct 18, 2016

Collaborator

not part of your change - but the import of DirectiveResolver above causes @angular/upgrade to depend on @angular/compiler so my AoT example is still getting the compiler at runtime, which makes the app big. Maybe a Closure Compiler issue but it's easier to trim the dependency.
@mhevery this suggests the aot version of upgrade should be a separate package so we can avoid the reflection dependencies?

@alexeagle

alexeagle Oct 18, 2016

Collaborator

not part of your change - but the import of DirectiveResolver above causes @angular/upgrade to depend on @angular/compiler so my AoT example is still getting the compiler at runtime, which makes the app big. Maybe a Closure Compiler issue but it's easier to trim the dependency.
@mhevery this suggests the aot version of upgrade should be a separate package so we can avoid the reflection dependencies?

This comment has been minimized.

@alexeagle

alexeagle Oct 18, 2016

Collaborator

upgrade -> platform-browser-dynamic -> compiler happens too

@alexeagle

alexeagle Oct 18, 2016

Collaborator

upgrade -> platform-browser-dynamic -> compiler happens too

@alexeagle alexeagle added cla: yes and removed cla: no labels Oct 19, 2016

@@ -23,7 +23,7 @@ module.exports = function(config) {
'node_modules/core-js/client/core.js',
// include Angular v1 for upgrade module testing
- 'node_modules/angular/angular.min.js',
+ 'node_modules/angular/angular.js',

This comment has been minimized.

@alexeagle

alexeagle Oct 19, 2016

Collaborator

can revert it?

@alexeagle

alexeagle Oct 19, 2016

Collaborator

can revert it?

+
+/** @experimental */
+export declare class UpgradeModule {
+ $injector: angular.IInjectorService;

This comment has been minimized.

@alexeagle

alexeagle Oct 19, 2016

Collaborator

this only works if you have this local angular1 typings to make it match.
Real users with angular 1 and angular 2 typings will find a structural mismatch here. Needs to be any

@alexeagle

alexeagle Oct 19, 2016

Collaborator

this only works if you have this local angular1 typings to make it match.
Real users with angular 1 and angular 2 typings will find a structural mismatch here. Needs to be any

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

We agreed to wait for it to break stuff

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

We agreed to wait for it to break stuff

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 19, 2016

feat(ngUpgrade): add support for AoT compiled upgrade applications
This commit introduces a new API to the ngUpgrade module, which is compatible
with AoT compilation. Primarily, it removes the dependency on reflection
over the Angular 2 metadata by introducing an API where this information
is explicitly defined, in the source code, in a way that is not lost through
AoT compilation.

This commit is a collaboration between @mhevery (who provided the original
design of the API); @gkalpak & @petebacondarwin (who implemented the
API and migrated the specs from the original ngUpgrade tests) and alexeagle
(who provided input and review).

This commit is an starting point, there is still work to be done:

* add more documentation
* validate the API via internal projects
* align the ngUpgrade compilation of A1 directives closer to the real A1
  compiler
* add more unit tests
* consider support for async `templateUrl` A1 upgraded components

Closes #12239
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 19, 2016

CLAs look good, thanks!

CLAs look good, thanks!

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 19, 2016

feat(ngUpgrade): add support for AoT compiled upgrade applications
This commit introduces a new API to the ngUpgrade module, which is compatible
with AoT compilation. Primarily, it removes the dependency on reflection
over the Angular 2 metadata by introducing an API where this information
is explicitly defined, in the source code, in a way that is not lost through
AoT compilation.

This commit is a collaboration between @mhevery (who provided the original
design of the API); @gkalpak & @petebacondarwin (who implemented the
API and migrated the specs from the original ngUpgrade tests) and @alexeagle
(who provided input and review).

This commit is an starting point, there is still work to be done:

* add more documentation
* validate the API via internal projects
* align the ngUpgrade compilation of A1 directives closer to the real A1
  compiler
* add more unit tests
* consider support for async `templateUrl` A1 upgraded components

Closes #12239
feat(ngUpgrade): add support for AoT compiled upgrade applications
This commit introduces a new API to the ngUpgrade module, which is compatible
with AoT compilation. Primarily, it removes the dependency on reflection
over the Angular 2 metadata by introducing an API where this information
is explicitly defined, in the source code, in a way that is not lost through
AoT compilation.

This commit is a collaboration between @mhevery (who provided the original
design of the API); @gkalpak & @petebacondarwin (who implemented the
API and migrated the specs from the original ngUpgrade tests) and @alexeagle
(who provided input and review).

This commit is an starting point, there is still work to be done:

* add more documentation
* validate the API via internal projects
* align the ngUpgrade compilation of A1 directives closer to the real A1
  compiler
* add more unit tests
* consider support for async `templateUrl` A1 upgraded components

Closes #12239
@@ -23,7 +23,7 @@ module.exports = function(config) {
'node_modules/core-js/client/core.js',
// include Angular v1 for upgrade module testing
- 'node_modules/angular/angular.min.js',
+ 'node_modules/angular/angular.js',

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery said it can stay un-minified

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery said it can stay un-minified

@@ -7,10 +7,11 @@
*/
export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

I think we are not using these constants in the new AoT code.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

I think we are not using these constants in the new AoT code.

+
+/** @experimental */
+export declare class UpgradeModule {
+ $injector: angular.IInjectorService;

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

We agreed to wait for it to break stuff

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

We agreed to wait for it to break stuff

+ upgrade.bootstrap(element, [ng1Module.name]);
+ return upgrade;
+ });
+}

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery - we should consider adding this helper to the public API

@petebacondarwin

petebacondarwin Oct 19, 2016

Member

@mhevery - we should consider adding this helper to the public API

@alxhub alxhub closed this in d6791ff Oct 19, 2016

btrigueiro added a commit to btrigueiro/angular that referenced this pull request Oct 21, 2016

feat(ngUpgrade): add support for AoT compiled upgrade applications
This commit introduces a new API to the ngUpgrade module, which is compatible
with AoT compilation. Primarily, it removes the dependency on reflection
over the Angular 2 metadata by introducing an API where this information
is explicitly defined, in the source code, in a way that is not lost through
AoT compilation.

This commit is a collaboration between @mhevery (who provided the original
design of the API); @gkalpak & @petebacondarwin (who implemented the
API and migrated the specs from the original ngUpgrade tests) and @alexeagle
(who provided input and review).

This commit is an starting point, there is still work to be done:

* add more documentation
* validate the API via internal projects
* align the ngUpgrade compilation of A1 directives closer to the real A1
  compiler
* add more unit tests
* consider support for async `templateUrl` A1 upgraded components

Closes #12239

@petebacondarwin petebacondarwin deleted the petebacondarwin:ngupgrade-aot branch Jul 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment