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

feat(elements): add custom element support, prototype AIO #22413

Closed

Conversation

@andrewseguin
Copy link
Contributor

commented Feb 23, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

Adds support for creating Custom Elements based on Angular Components. Makes changes to angular.io so that it uses custom elements instead of manually creating components.

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[] Other... Please describe:

What is the new behavior?

Can create custom elements based on Angular Components.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@andrewseguin andrewseguin requested a review from IgorMinar Feb 23, 2018

@googlebot googlebot added the cla: no label Feb 23, 2018

@andrewseguin andrewseguin force-pushed the andrewseguin:aio-elements-slimmed-ng-element branch 2 times, most recently from 6b1a5d8 to 2bc0c3c Feb 26, 2018

@andrewseguin andrewseguin added cla: yes and removed cla: no labels Feb 26, 2018

@andrewseguin andrewseguin force-pushed the andrewseguin:aio-elements-slimmed-ng-element branch 3 times, most recently from 8bf2b3e to 2d8893a Feb 26, 2018

@angular angular deleted a comment from googlebot Feb 26, 2018

@andrewseguin andrewseguin force-pushed the andrewseguin:aio-elements-slimmed-ng-element branch from 2d8893a to d72c281 Feb 27, 2018

@IgorMinar
Copy link
Member

left a comment

I'm submitting all the comments I have so far. I'm still reviewing this PR...

@@ -75,6 +75,7 @@
"@angular/common": "^5.2.0",
"@angular/compiler": "^5.2.0",
"@angular/core": "^5.2.0",
"@angular/elements": "^5.2.0",

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

isn't this going to fail because we don't have elements released at ^5.2.0?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Yes, this is definitely not right. Should I put 6.0.0 for this line?

@@ -40,44 +38,38 @@ describe('CodeComponent', () => {
delete (window as any)['prettyPrintOne'];
});

beforeEach(() => {
beforeEach(async(() => {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

why async?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

No need, can be removed. It got added at some point as I was refactoring the components and it looks like at some point it became unnecessary again.

@@ -87,25 +79,33 @@ describe('CodeComponent', () => {
it('should add line numbers to one-line code sample when linenums set true', () => {
hostComponent.linenums = 'true';
fixture.detectChanges();
hostComponent.refresh();

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

note to self: lookup what this does refresh does. it's odd that we have to call it everywhere.

}

/** Updates the code component with the latest state set on the host component. */
refresh() {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

ah. I see. you could have implemented this via ngOnChages and then you wouldn't need to sprinkle the refresh all over the place. have you considered that?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

I did, this one is particularly tricky. Components that use the CodeComponent do not know what the code Input should be until after their view is checked. I'd love to make code an @Input but it means we add another round of change detection for the CodeComponent to receive the code, which ends up making it confusing if it should show the "Missing code" text.

I made some changes to make this a bit clearer/better:

  • I added ngOnChanges to the code component so that inputs automatically update the code (less refresh calls)
  • Made it clearer that the tests need to explicitly set the code on the CodeComponent just like normal clients would
export const ELEMENT_MODULE_PATHS_AS_ROUTES = [
{
selector: 'aio-announcement-bar',
loadChildren: './announcement-bar/announcement-bar.module#AnnouncementBarModule'

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

originally this one component was intentionally not lazy loaded because it is used on the homepage which we want to ensure that renders correctly even when offline and also we want to render if whole as soon as possible and without reflows because it's a home page.

it would be good to get a preview build out of this PR so that we can see how lazy loading impacts the user experience.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Mar 16, 2018

Member

we should revisit this one after this PR is merged. I believe it's going to be a problem.

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Mar 16, 2018

Author Contributor

Sure thing, will add it to our task-list

@Injectable()
export class ElementsLoader {
/** Map of unregistered custom elements and their respective module paths to load. */
unregisteredElements: Map<string, string>;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

"unregistered" means "no longer registered" no? wouldn't a better name be "notYetRegisteredElements" or "elementsToRegister" or "elementsToBeRegistered"?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

when you look at it more closely, it's actually "elementsToLoad" or "modulesToLoad"

ElementsLoader,
{ provide: NgModuleFactoryLoader, useClass: FakeModuleFactoryLoader },
{ provide: ELEMENT_MODULE_PATHS_TOKEN, useValue: new Map([
['element-a-selector', 'element-a-module-path']

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

crazy idea - how about to DRY up this mapping by inferring the path from the selector or vice versa?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Would require a bit more magic than I'd like - here's an example of a real mapping:

selector: aio-contributor-list
module: ./contributor/contributor-list.module#ContributorListModule

The folder is just contributor, the module file is contributor-list, the selector is aio-contributor-list.

We could try to get things lined up and we'll have to deal with the code folder which contains two modules/elements


// Providing these routes as a signal to the build system that these modules should be
// registered as lazy-loadable.
// TODO(andrewjs): Provide first-class support for providing this.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

you no longer need to use the router to code split. see: angular/angular-cli#9515 (might require cli update though since this is recent)

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Awesome, looking forward to making this change. Tried to get latest version of CLI but getting some errors. Will make a note to change this.

* the browser. Custom elements that are registered will be removed from the list of unregistered
* elements so that they will not be queried in subsequent calls.
*/
loadContainingCustomElements(element: HTMLElement): Observable<any> {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

what is the return type? any is odd. it seems to be an array of undefines if I'm reading the code correctly. Maybe just cast it to Observable<null> just to make it clear that we don't propagate any values through this stream. it's just a signaling stream.

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Yup good call, it's a signal to the callee so Observable<null> works well and doesn't imply its bringing back any information.

@@ -11,15 +11,16 @@ const { API_SOURCE_PATH } = require('../config');

const packageMap = {
animations: ['animations/index.ts', 'animations/browser/index.ts', 'animations/browser/testing/index.ts'],
common: ['common/index.ts', 'common/testing/index.ts'],
common: ['common/index.ts', 'common/testing/index.ts', 'common/http/index.ts', 'common/http/testing/index.ts'],

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

whaaaat? how were we able to previously generate api docs for the http client without this? @petebacondarwin do you know what's going on here?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 27, 2018

Member

Note that this is only in the "author's" package, which is what gets run when we are attempting to watch and partially generate docs for the author workflow.

The main doc-gen does not rely upon this mapping.

So the answer is that normal doc-gen works but an author working on these local files would not have seen the partial build triggered.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

aha. but why do we have two of these lists then? can't we just have one?

@IgorMinar
Copy link
Member

left a comment

check out the API proposal burried in one of the comments in the ElementLoader class.


// Modules containing custom elements must be set up as lazy-loaded routes (loadChildren)
// TODO(andrewjs): This is a hack, Angular should have first-class support for preparing a module
// that contains custom elements.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

no longer needed. see my comment above.

}

/** Registers the custom element defined on the WithCustomElement module factory. */
register(selector: string) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

should this be private?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

or is it left as is because of tests?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

It can be private - I considered that it's possible we want this to be another entry-point to the service but it's not needed right now.

const resolver = moduleRef.componentFactoryResolver;
const customElement = moduleRef.instance.customElement;

return resolver.resolveComponentFactory(customElement);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

uhh.. this is quite a round-about way of getting the component factory

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Mar 2, 2018

Author Contributor

Changed this so the user doesn't have to do this anymore

private getElementComponentFactory(
moduleRef: NgModuleRef<WithCustomElement>): ComponentFactory<string> {
const resolver = moduleRef.componentFactoryResolver;
const customElement = moduleRef.instance.customElement;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

this "customElement" is not a custom element at all, it's the Angular component class. we need to be more consistent about the terminology used because with this many levels of indirections a slip-up like this can easily make the whole code incomprehensible.

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Oh yeah for sure, this should be renamed. How about customElementComponent?

const elementModuleRef = elementModuleFactory.create(injector);
const componentFactory = this.getElementComponentFactory(elementModuleRef);

const ngElementConfig = getConfigFromComponentFactory(componentFactory, injector);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

why this extra indirection? why not just:

const NgElement = createNgElementConstructor(componentFactory, injector);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

previously you had the signature look like what I suggested, with the additional param being strategy. I kind of liked that.

there is however one more option:

@Injectable({
  scope: APP_ROOT_SCOPE
})
export class ElementsLoader {
  constructor(private ngElementer, private injector) {
  }

  register(selector: string) {
    const modulePath = this.unregisteredElements.get(selector)!;
    return this.moduleFactoryLoader.load(modulePath).then(elementModuleFactory => {
      if (!this.unregisteredElements.has(selector)) { return; }
      const elementModuleRef = elementModuleFactory.create(this.injector);
      const componentClass = elementModuleRef.instance.customElement;
      const componentFactory = elementModuleRef.resolveComponentFactory(componentClass);
      const ngElementClass = this.ngElementer(componentClass, {componentFactory});
      customElements!.define(selector, ngElementClass);
  }
}

now the advantage of this this api is that this.ngElementer(componentClass, {componentFactory}); could be turned into this.ngElementer(componentClass); without a breaking change. And this is the api that we'll one once ivy is live.

the "options" object, could then also be extended to provide the strategy we discussed before.

what do you think?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

This design is what I showed except that it has a bit more added to the config (related to attribute/properties on the element). In your example, you'll still need to provide the injector to the NgElementer and I'm not sure it provides enough flexibility for the user to change the logic used for creation/destruction/input changes.

There's three things that the NgElement ultimately wants to know:

  • ObservedAttributes - What attributes do you want the element to listen to? When those attributes change, which property should be set? For example, consider a component with @Input('prefixedFooFoo') fooFoo - we want to be notified on the element's attribute changes. Options include: foo-foo, prefixed-foo-foo, foofoo, etc.
  • Property Proxies - Which properties should show up on the element and proxy to the component?
  • Underlying strategy - How/when is the component created/destroyed? What does change detection look like when inputs change?

We can capture these aspects by requesting the user to provide an NgElementConfig with the following signature:

export interface NgElementConfig {
  attributeToPropertyInputs: Map<string, string>;
  propertyInputs: string[];
  strategyFactory: NgElementStrategyFactory;
}

[In the future we will want to add more to this config, e.g. which functions should be exposed on the element and proxy to the component ref]

Since the config has no knowledge of ComponentFactory, we are safe from breaking changes in Ivy. In fact, NgElement is totally agnostic to any Angular-specific concepts - it simply relays information to the strategy.

In our current world of ComponentFactory, we want to provide a reliable opinionated default so that users don't need to worry about creating this config themselves. This is encapsulated in the component-factory-strategy.ts which contains the createConfigFromComponentFactory method which handles all the heavy lifting for the user. The generated config uses the following logic:

  • Observed attributes will be the kebab-cased versions of the inputs set on the factory
  • All inputs will be set as proxy properties on the element
  • The underlying strategy uses the component factory and injector to create new components.

Note that in the future we can provide other config-generators (probably an Ivy-specific one). This provided one is intentionally blunt about it using ComponentFactory so that later we can deprecate it safely without worrying about making breaking changes to it for Ivy.

If the user doesn't agree with the defaults, they can modify them or build their own config (and even create their own strategy).

I think this strikes a good balance of providing an easy default path for users to quickly use custom elements but also gives them flexibility to change practically any part of it.

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Mar 2, 2018

Author Contributor

Okay slight change

  • Added injector into the config
  • Changed it so the user provides the component, not factory (can get this from the injector)
  • Removed property proxies from the config
  • User doesn't need to create the config anymore

The interface is much simpler now and reflects I think our goal - the user creates the class and then registers it (no more fussing with a config). If they want to customize then they are able to do so with properties in the config argument but not necessary.

/** Gets the component factory of the custom element defined on the NgModuleRef. */
private getElementComponentFactory(
moduleRef: NgModuleRef<WithCustomElement>): ComponentFactory<string> {
const resolver = moduleRef.componentFactoryResolver;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

I'd expect you to inject ComponentFactoryResolver rather than peal it off of the ngModuleRef. for some reason I'd find it less surprising that way. may be just my style preference - I like to see all dependencies injected so that it's easy to understand what this class uses.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

ah. I'm wrong. this is a resolver specific to this NgModuleRef. the reason why I got confused is that you have this.moduleRef and moduleRef in the same scope but they are two different NgModuleRefs. Perphaps rename one of them to prevent the confusion?

yarn.lock Outdated
@@ -258,21 +258,17 @@ amdetective@0.0.2:
dependencies:
esprima "~1.2.2"

"angular-1.5@npm:angular@1.5":

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 27, 2018

Member

this whole change looks suspicious. you are essentially removing AngularJS v1.6 which we need for compatibility testing. can you explain what are you trying to do please?

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 27, 2018

Author Contributor

Not sure what's generating the lock this way, I'm probably missing something from the process. The CI seemed to indicate that the yarn lock was stale, so I ran yarn and exported the change to my lock hoping it'll run CI and produce a running instance of AIO.

This comment has been minimized.

Copy link
@andrewseguin

andrewseguin Feb 28, 2018

Author Contributor

Fixed the package.json - looks like it was a bad rebase that caused this

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

@andrewseguin - I would suggest that the commit that updates the AIO application is more of a feat(aio) or refact(aio) commit not docs(aio), which should really only be used for changes to the documentation of the AIO app or build infrastructure itself.

@petebacondarwin petebacondarwin added this to WIP in docs-infra Feb 28, 2018

@jponeill

This comment has been minimized.

Copy link

commented Feb 28, 2018

Should there be separate PRs: one for aio and one for the elements package? Maybe #21939 should be reopened? If possible, would like the elements package merged even if aio isn't ready.

@andrewseguin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

Up to Igor - he wanted a PR with both the feature and AIO together. Not sure if this will be how it ends up

@andrewseguin andrewseguin force-pushed the andrewseguin:aio-elements-slimmed-ng-element branch from e47a2c8 to 3ca66da Feb 28, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

mhevery added a commit that referenced this pull request Mar 16, 2018

@petebacondarwin petebacondarwin removed this from WIP in docs-infra Mar 23, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.