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

docs(compiler): initial outline of Ivy Compiler Architecture design doc #22916

Merged
merged 6 commits into from Mar 30, 2018

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Mar 21, 2018

No description provided.

@mary-poppins
Copy link

You can preview c8c95c5 at https://pr22916-c8c95c5.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4beaeac at https://pr22916-4beaeac.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ca4beef at https://pr22916-ca4beef.ngbuilds.io/.

GreetComponent,
'greet',
{input: 'input'}
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

These information is not enough to support class inheritance without constructor override (in AOT), due to the @Inject() usage, the constructor signatures will not reflect real deps, like:

class MyComp {
  constructor(@Inject(DOCUMENT) doc: any) {}
}

Will only generate (.d.ts):

class MyComp {
  constructor(doc: any) {}
}

So there must be a separate record for ctorParameters in metadata. (Relates to angular/tsickle#760)

For example, use a tuple type like:

import { DOCUMENT } from '@angular/common';

class MyComp {
  static ngComponentDef: i0.NgComponentDef<
    MyComp,  
    'my-comp', 
    {},
    [DOCUMENT]
  >;
}

But the real problem is, the relative path in declaration may different to path in emitted js, with ts file:

import { MyToken } from './my-token';

class MyComp {
  constructor(token: MyToken) {}
}

Which naturally generates:

import { MyToken } from './my-token';

class MyComp {
  static ngComponentDef: i0.NgComponentDef<
    MyComp,  
    'my-comp', 
    {},
    [MyToken]
  >;
}

But the AOT compiler must not depend on the relative import path.

In current version of metadata.json there's a re-exporting procedure like (renaming omitted):

{
  "metadata": {
    "MyComp": {
      "__symbolic": "class",
      "members": {
          "__ctor__": [
            {
              "__symbolic": "constructor",
              "parameters": [
                {
                  "__symbolic": "reference",
                  "name": "MyToken"
                }
              ]
            }
          ]
       }
    }
  },
  "origins": {
    "MyComp": "./src/my-comp",
    "MyToken": "./src/my-token"
  }
}

It works because there's a centralized path-mapping.

But in the new .d.ts-based metadata, everything is distributed, so the metadata collector has no way to know where to get the MyToken reference locally. So the metadata collector (when compiling the app or lib consuming another) needs to traverse all the declaration files to generates the mapping (for all libs), would that still be performance friendly?

Copy link
Contributor

Choose a reason for hiding this comment

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

The constructor information is not necessary to record in the .d.ts file since class contains the factory in the ngComponentDef. In the case you presented you would get,

class MyComp {
  static ngComponentDef = defineComponent({
    ...
    factory: function MyComp_Factory() {
      return new MyComp(inject(DOCUMENT));
    },
    ...
  );
}

The fundamental change here is that the factory is generated in the same file as the class (instead in an external factory file) so we don't need to record any information about it in the .d.ts file.

Copy link
Contributor

@trotyl trotyl Mar 27, 2018

Choose a reason for hiding this comment

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

@chuckjaz So does AOT compiler (when compiling the app) need to parse the library JavaScript files now?

What I meant here is one could extend a library class:

import { LibComp } from 'my-lib';

// Given:
// @Component({})
// export class LibComp {
//   constructor(libService: LibService) { }
//   
//   static ngComponentDef = defineComponent({
//     factory: function LibComp_Factory() { return new LibComp(inject(LibService)) }
//   })
// }

@Component({})
export class MyComp extends LibComp { }

Even LibComp already have a compiled factory, how could that be used by MyComp? Namely how to generate:

import { LibComp, LibService } from 'my-lib';
// exportName for LibService may actually be different or auto-generated

@Component({})
export class MyComp extends LibComp {
  static ngComponentDef = defineComponent({
    factory: function MyComp_Factory() { return new MyComp(inject(LibService)) }
  })
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I understand you point now. I will discuss with @mhevery and @alxhub.

Copy link
Contributor

@mhevery mhevery Mar 27, 2018

Choose a reason for hiding this comment

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

Per our discussion with @chuckjaz The above problem can be solved by allowing the invocation of the parent class's factory with a custom constructor.

export class LibComp {
  constructor(libService: LibService) { }
  
  static ngComponentDef = defineComponent({
    factory: function LibComp_Factory(Constructor: Type<any> = LibComp) { 
      return new Constructor(inject(LibService));
    }
  })
}

export class MyComp extends LibComp { 
  static ngComponentDef = defineComponent({
    factory: function MyComp_Factory(Constructor: Type<any> = MyComp) { 
      return LibComp.ngDefineConstructor.factory(Constructor);
    }
  })
}

Copy link
Contributor

@trotyl trotyl Mar 28, 2018

Choose a reason for hiding this comment

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

@mhevery Then would it better to separate factory from ngXXXDef? The type of derived class could be completely different than base class. Otherwise it would need to check all of the possible ngComponentDef, ngDirectiveDef, ngPipeDef, ngInjectorDef...

Also would it better to use an INHERIED token rather than hard-coded base class name? So one could use dynamic expression as base class, which is an already known limitation today and Ivy may be the best chance to improve.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be be better to have a runtime helper here something like:

export class MyComp extends LibComp { 
  static ngComponentDef = defineComponent({
    ...
    factory: factoryFrom(MyComp),
    ...
  })
}

and if the extends is not a simple identifier then the class can be rewritten:

const a = addTrait(LibComp);
export class MyComp extends a {
  static ngComponentDef = defineComponent({
    ...
    factory: factoryFrom(a),
    ...
  });
}

@mary-poppins
Copy link

You can preview f45692c at https://pr22916-f45692c.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 763f439 at https://pr22916-763f439.ngbuilds.io/.

@mary-poppins
Copy link

You can preview b8982ce at https://pr22916-b8982ce.ngbuilds.io/.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 30, 2018
@alxhub alxhub merged commit d6bf71a into angular:master Mar 30, 2018
@alxhub alxhub deleted the ivy-design branch March 30, 2018 22:43
@trotyl trotyl mentioned this pull request Apr 24, 2018
@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 13, 2019
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: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants