feat(elements): implement `@angular/elements` #19469

gkalpak · 2017-09-28T23:26:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] 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 current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

mary-poppins · 2017-09-30T08:44:11Z

You can preview 3e825a1 at https://pr19469-3e825a1.ngbuilds.io/.

mhevery

Few comments. @robwormald will have more feedback for you.

Also I think we decided to cal it just @angular/elements so you will need to do search and replace.

mhevery · 2017-10-03T13:56:01Z

packages/custom-elements/src/ng-element-constructor.ts

+  // (i.e. not transpiled to an ES5 constructor function).
+  // TODO(gkalpak): Document that if you are using ES5 sources you need to include a polyfill
+  //                (e.g. https://github.com/webcomponents/webcomponentsjs/blob/master/custom-elements-es5-adapter.js).
+  class NgElementConstructorImpl extends NgElementBaseImpl<T> {


Why do we have FooImpl? Why not just Foo. There is but one implementation.

See: https://github.com/angular/angular/blob/master/docs/NAMING.md

Not sure what you mean. There are interfaces called NgElementConstructor and NgElementBase, so I suffixed the implementations with Impl (note that all Impl-suffixed things are not public API).

Open to suggestions for a different naming scheme 😃

mhevery · 2017-10-03T13:57:49Z

packages/custom-elements/src/ng-element.ts

+ * TODO(gkalpak): Add docs.
+ * @experimental
+ */
+export interface NgElementBase<T> extends HTMLElement {


I don't think this interface adds value. Just go directly to implementation skip interface and remove -Impl

The implementation has public members (properties, methods, constructor) that I don't want to document.

BTW, I was thinking about renaming these as follows:

// Before: NgElement = NgElementBase & {[prop in keyof P]: P[prop]} NgElementBase { /* the actual interface without custom props */ } // After: NgElementWithProps = NgElement & {[prop in keyof P]: P[prop]} NgElement { /* the actual interface without custom props */ }

Would it be clearer/less clattered?

mhevery · 2017-10-03T13:58:32Z

packages/custom-elements/src/ng-element.ts

+  disconnected,
+}
+
+interface NgElementBaseImplConnected<T> extends NgElementBaseImpl<T> {


Interface by definition can't be Impl

mhevery · 2017-10-03T14:00:06Z

packages/custom-elements/test/ng-element_spec.ts

+import {NgElementBaseImpl, NgElementInput, NgElementOutput} from '../src/ng-element';
+import {setScheduler} from '../src/utils';
+
+export function main() {


main used to be for Dart. It is no longer needed, you can skip it.

Hm...not so sure about that 😉

mary-poppins · 2017-10-03T21:43:49Z

You can preview 27efe36 at https://pr19469-27efe36.ngbuilds.io/.

robwormald · 2017-10-03T21:59:49Z

packages/custom-elements/package.json

@@ -0,0 +1,22 @@
+{
+  "name": "@angular/custom-elements",


think we're going with @angular/elements here

robwormald · 2017-10-03T22:02:33Z

packages/custom-elements/src/extract-projectable-nodes.ts

+
+// NOTE: This is a (slightly improved) version of what is used in ngUpgrade's
+//       `DowngradeComponentAdapter`.
+// TODO(gkalpak): Investigate if it makes sense to share the code.


do we want to support this case at all? it seems like it should be unsupported, since we're not going to be able to get any of the @ContentChild/Children stuff to work here?

<slot> handles this natively, what's the benefit of doing this over relying on platform behavior?

I guess ContentChild(ren) don't work with projected content. What else doesn't work?

I don't feel strongly about supporting it. In my head, the idea is to have something work as similar as possible when inside and outside an Angular app (so I can re-use components are "native" Angular components or custom elements).

robwormald · 2017-10-03T22:03:26Z

packages/custom-elements/src/ng-element-constructor.ts

+  readonly selector: string;
+  readonly observedAttributes: string[];
+
+  upgrade(host: HTMLElement): NgElement<T, P>;


is this your method name? Worried about a potential conflict with a future native upgrade thing which has been discussed

Yeah. I thought it makes sense to call it something that describes what it is actually doing (in CustomElements terms). In the design docs it is called attach() (which is more arbitrary).

I wasn't aware of any discussions about a native upgrade thing. Are there any resources you can point me to?

robwormald · 2017-10-03T22:07:26Z

packages/custom-elements/src/ng-element-constructor.ts

+export function createNgElementConstructor<T, P>(appRef: ApplicationRef, injector: Injector, componentFactory: ComponentFactory<T>): NgElementConstructorInternal<T, P> {
+  const inputs = componentFactory.inputs.map(({propName, templateName}) => ({
+    propName,
+    attrName: camelToKebabCase(templateName),


This is kinda sketchy - seems like we should avoid these heuristics where possible?

The problem this is trying to address is that most of the time attribute names are the same as property names (unless explicitly overriden), which in turn are usually camelCased. CamelCased attributes are fine when something is compiled with Angular's compiler, but we can't have them in native HTML.

So, we either need to lowercase them or kebabcase them. (@mhevery said kebabcasing is fine.)

robwormald · 2017-10-03T22:08:02Z

packages/custom-elements/src/ng-element-constructor.ts

+  //                (e.g. https://github.com/webcomponents/webcomponentsjs/blob/master/custom-elements-es5-adapter.js).
+  class NgElementConstructorImpl extends NgElementBaseImpl<T> {
+    static readonly selector = componentFactory.selector.toLowerCase();
+    static readonly observedAttributes = inputs.map(input => input.attrName);


this seems to conflate attributes and @inputs ?

Not sure I understand the question, but probably yes 😁
According to the design, updating an attribute will propagate to the corresponding property (not the other way around though) and attributes will be considered for initializing properties (when we instantiate the component).

robwormald · 2017-10-03T22:10:52Z

packages/custom-elements/src/ng-element-constructor.ts

+  // TODO(gkalpak): Document that if you are using ES5 sources you need to include a polyfill
+  //                (e.g. https://github.com/webcomponents/webcomponentsjs/blob/master/custom-elements-es5-adapter.js).
+  class NgElementConstructorImpl extends NgElementBaseImpl<T> {
+    static readonly selector = componentFactory.selector.toLowerCase();


should be static is = componentFactory.selector.toLowerCase(); - matches with Polymer nicely?

Why are we lowercasing the selector? We should not allow the following usages:

foo-Bar - (case sensitive)

foobar - violates the CE spec

[foo-bar] - doesn't make sense in CE land

(tldr, we should require people to always use dash-case selectors, because the spec requires it :)

Happy to rename selector to is (although I think selector is more clear 😁).
The lowercasing is just to be consistent with the casing, so that we can use it to match against nodeNames later without having to lower/uppercase it every time.

I have also thought about "validating" the selector and throwing if it is not a dash-case selector. In practice, I don't think it is possible to instantiate an element that hasn't been added to the customElements registry, which means that customElements.define(selector, ...) will throw anyway.

But maybe it will be possible to bypass the customElements registry in the future, so 👍 for having a explicit check for the selector.

robwormald · 2017-10-03T22:12:06Z

packages/custom-elements/src/ng-element-constructor.ts

+    static readonly selector = componentFactory.selector.toLowerCase();
+    static readonly observedAttributes = inputs.map(input => input.attrName);
+    static readonly onConnected = new EventEmitter<NgElement<T, P>>();
+    static readonly onDisconnected = new EventEmitter<NgElement<T, P>>();


what are these for? can we not just use the lifecycle hooks themselves?

These emit values when an NgElement of this type is connected/disconnected.
How would we use the lifecycle hooks for that?

mary-poppins · 2017-10-16T22:21:15Z

You can preview 6a7a490 at https://pr19469-6a7a490.ngbuilds.io/.

trotyl · 2017-10-17T01:32:39Z

karma-js.conf.js

@@ -34,6 +34,9 @@ module.exports = function(config) {
      'node_modules/zone.js/dist/async-test.js',
      'node_modules/zone.js/dist/fake-async-test.js',

+      // Include polyfills necessary for testing the `custom-elements` package.


Shouldn't it be elements package now?

Good catch 😃

mary-poppins · 2017-11-02T00:40:43Z

You can preview ead31ec at https://pr19469-ead31ec.ngbuilds.io/.

mary-poppins · 2017-11-02T10:13:09Z

You can preview 21bfaf2 at https://pr19469-21bfaf2.ngbuilds.io/.

vicb · 2017-11-02T22:27:35Z

@gkalpak is it expected to have a feat on a patch branch (5.0.x) ?

gkalpak · 2017-11-02T22:38:27Z

Hm...probably not. I had it marked as master-only before the release, but since it didn't get merged, I changed to master & patch so that it lands on the branches that it would have landed if it were merged earlier. Didn't think about the fix-vs-feat distinction.
(I've had too much AngularJS, I guess, where patch releases are treated as minor releases (since there can be no major version bumps) 😁)

Thx for catching this 👍

vicb · 2017-11-02T23:08:16Z

Thanks for you PR!
Target labels are confusing.

gkalpak · 2017-11-03T09:33:02Z

🎉

This PR was merged without API docs and general rollout plan. We can't release this as is in 5.1 without a plan for documentation, cli integration, etc.

MarkPieszak · 2017-11-04T14:15:08Z

Was this merged in elsewhere ? @gkalpak

IgorMinar · 2017-11-04T15:34:46Z

@MarkPieszak I moved it to https://github.com/angular/angular/tree/labs/elements

jponeill · 2017-12-06T14:34:31Z

Should this be reopened or is there some other way to see progress on this feature? (I checked the lab branch and it doesn't look like anything has changed since the backed out merge.)

eggp · 2017-12-07T12:23:43Z

it is a live feature ? it will be in the future ? :) last commit in: https://github.com/angular/angular/tree/labs/elements oct.12 :(

gkalpak · 2017-12-08T09:06:17Z

Not sure what you mean by "live feature". It is not out of labs yet (so not part of the actual release) if that is what you mean. Things under labs are experimental and not guaranteed to make it into core (at least that's my understanding), but I am pretty confident that "Angular Components as Custom Elements" will definitely make it into core - one way or another.

In the meantime, we are investigating some internal core changes that will also improve ngElements (e.g. make it possible to reduce the bundle size of bundled components, etc). And we might soon try out ngElements on specific parts of angular.io. Docs are also on the roadmap not too far down the road 😃

Stay tuned! And feel free to play around with labs/elements and give us feedback :heart:
_{(Some useful resources for getting started can be found in #20859.)/sub>}

angular-automatic-lock-bot · 2019-09-12T23:52:15Z

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.}

googlebot added the cla: yes label Sep 28, 2017

gkalpak added state: WIP target: major This PR is targeted for the next major release feature Issue that requests a new feature labels Sep 28, 2017

gkalpak force-pushed the feat-custom-elements branch 3 times, most recently from 1e27a28 to 3e825a1 Compare September 30, 2017 08:34

mhevery reviewed Oct 3, 2017

View reviewed changes

gkalpak force-pushed the feat-custom-elements branch from 3e825a1 to 27efe36 Compare October 3, 2017 21:34

robwormald reviewed Oct 3, 2017

View reviewed changes

gkalpak force-pushed the feat-custom-elements branch 2 times, most recently from 443e5d7 to ea73465 Compare October 4, 2017 13:18

gkalpak changed the title ~~WIP - feat(custom-elements): implement custom-elements~~ WIP - feat(elements): implement @angular/elements Oct 4, 2017

gkalpak force-pushed the feat-custom-elements branch 5 times, most recently from 945fff2 to 663e924 Compare October 11, 2017 22:50

gkalpak force-pushed the feat-custom-elements branch 4 times, most recently from a9a81a1 to 6a7a490 Compare October 16, 2017 22:08

trotyl reviewed Oct 17, 2017

View reviewed changes

gkalpak force-pushed the feat-custom-elements branch 4 times, most recently from 6189089 to caaa98e Compare October 17, 2017 11:20

gkalpak added 4 commits November 2, 2017 11:49

feat(elements): implement NgElement

c1c9083

feat(elements): implement NgElementConstructor

4d42d9a

feat(elements): implement NgElements

9452693

feat(elements): implement registerAsCustomElements()

21bfaf2

gkalpak force-pushed the feat-custom-elements branch from ead31ec to 21bfaf2 Compare November 2, 2017 09:49

gkalpak added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Nov 2, 2017

vicb added the state: blocked label Nov 2, 2017

gkalpak added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release state: blocked labels Nov 2, 2017

vicb closed this in dcf8840 Nov 2, 2017

gkalpak deleted the feat-custom-elements branch November 3, 2017 09:33

IgorMinar added a commit that referenced this pull request Nov 3, 2017

revert: feat(elements): implement @angular/elements #19469

b8af122

This PR was merged without API docs and general rollout plan. We can't release this as is in 5.1 without a plan for documentation, cli integration, etc.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

feat(elements): implement @angular/elements #19469

feat(elements): implement @angular/elements #19469

Conversation

gkalpak commented Sep 28, 2017

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Sep 30, 2017

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Nov 2, 2017

mary-poppins commented Nov 2, 2017

vicb commented Nov 2, 2017

gkalpak commented Nov 2, 2017

vicb commented Nov 2, 2017

gkalpak commented Nov 3, 2017

MarkPieszak commented Nov 4, 2017

IgorMinar commented Nov 4, 2017

jponeill commented Dec 6, 2017

eggp commented Dec 7, 2017

gkalpak commented Dec 8, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

feat(elements): implement `@angular/elements` #19469

feat(elements): implement `@angular/elements` #19469