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

Adapters for new custom elements API and Angular (2+) #6

Merged
merged 17 commits into from
Feb 12, 2018

Conversation

alterx
Copy link
Collaborator

@alterx alterx commented Feb 14, 2017

I added a couple of things here:

  • Changed a bit the way the custom elements v0 adapter works and stated that this API is deprecated.
  • Added an adapter for the new (v1) API
  • Added instructions on how to use both adapters to enable nanocomponents in Angular (via custom elements)

The v1 API adapter is intentionally written using the new ES2015 class syntax since it won't work with the old ES5 way. I shouldn't affect users since "every browser that implements custom elements also implements class syntax."
Also, see this comment.

- Fixed examples for custom elements v0
- Added custom elements v1 examples
- Added Angular example and notes

Signed-off-by: Carlos Vega <[email protected]>
- Adding new v1 adapter
- Fixing a couple of bugs in v0 adapter
- Renaming files

Signed-off-by: Carlos Vega <[email protected]>
@alterx alterx mentioned this pull request Feb 14, 2017
README.md Outdated
```

## Angular
```js

Choose a reason for hiding this comment

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

I know this is the angular way, but can we show the es3/es5 equivalent to match other examples ?

Copy link
Collaborator Author

@alterx alterx Feb 23, 2017

Choose a reason for hiding this comment

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

We can definitely show the es6 version of it. Let's keep in mind that the preferred way of developing Angular 2 applications it's using TypeScript and that for Custom Elements v2 you need to use the new class syntax for it to work. Prototypal inheritance will just not work.
Angular it's really different from the other frameworks (preact, react, et al), it's not just Javascript (TM)

Choose a reason for hiding this comment

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

For sure, understood (and also enraged by the decision to only use classes for custom elements)

But (and @yoshuawuyts can correct me here if I'm wrong) - my understanding of nanocomponent is that
it's a generic interface to create components, in the same way

In the custom-elements v1 I can see that approach laid out plainly, here I can't see how it relates to using nanocomponent

(on a side note it looks like there are ways to hack around the class restriction - https://github.com/webcomponents/custom-elements/blob/master/src/native-shim.js#L98-L153)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the adapter provides that generic interface. The Angular (2+) example uses the Custom Element produced and integrates it into its template. In that way, the angular example shouldn't be generic. Should be specific on how to let know Angular about your custom element. In this case you only need to use the CUSTOM_ELEMENTS schema and provide it to your module.

And yeah, there are ways but sticking to the standard is always better. You can still use v0 if you don't want to use the class syntax. Also, every browser that implements custom elements also implements the class syntax.

README.md Outdated
});
})(window.app || (window.app = {}));


/*
You can now use the custom element in either a standalone or inline template

Choose a reason for hiding this comment

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

very cool vanilla js example - I guess where I'm confused is this: at what point do we pass the component to the render property of the options object passed to nanocomponent

Copy link
Collaborator Author

@alterx alterx Feb 23, 2017

Choose a reason for hiding this comment

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

Yeah, the cool thing about Angular is that once you've added the CUSTOM_ELEMENTS schema that's it. The only thing you need to do is use your custom element as a regular HTML tag in your angular template as stated in the last part of the example:

<custom-widget [attr.title]="title">

Choose a reason for hiding this comment

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

so we're saying... this doesn't need nanocomponent at all?

or is there a way to shoehorn angular into the nanocomponent concept?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does need nanocompoent. You just use the nanocomponent to custom element adapter and that's it. Once you've registered your nanocomponent as a custom element and added to CUSTOM_ELEMENTS schema to your angular module, you're done and you can use the nanocomponent as a custom element inside any angular template.

It looks something like this:
https://snag.gy/NJxhLy.jpg

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And then, in your template:
https://snag.gy/0Ra4mj.jpg

Choose a reason for hiding this comment

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

ok I get you now - I think the readme should try to clarify that, although I understand it's complex to convey -

that's a whole lot of boilerplate though (and I realise Angular is all about that) - but I'm still wondering whether it's possible to bypass it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that boilerplate is completely Angular related. Every Angular application will already have at least one NgModule.

So, the steps are:

  1. Use the Custom Element adapter and register the element (which is already available)
  2. Literally, add 2 lines to the already existing NgModule in your application.
  3. You're ready to go since Angular is able to deal with custom elements natively.

I really can't see a more straightforward process than that. The other option would be to implement a directive, component or service but that would only add more complexity since now you'd have to provide the component, make the service injectable, etc (all of this is Angular related) which would also mean more boilerplate to set up properly.

With the current approach, we leverage Angular native capabilities and embrace the standard, hence future-proofing our current solution.

I guess the main issue is that Angular is not like any of the other frameworks in the Readme, we're not even "registering" the component because there's no need to. We can just leverage the Custom Element adapter and we're ready to go.

@davidmarkclements
Copy link

davidmarkclements commented Feb 23, 2017

So, each of the examples in the readme shows the full path from element creation to ultimate attachment to the DOM. In some cases, we need multiple files - the Elm example for instance.

In this case, as I understand it - we need 5 files

index.html

<body>
  <app></app>
</body>

main.ts

import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {AppModule} from './app.module';

platformBrowserDynamic().bootstrapModule(AppModule);

app.module

import { BrowserModule } from '@angular/platform-browser';
import { NgModule, CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
import { AppComponent } from './app.component';

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    BrowserModule
  ],
  providers: [],
  schemas: [ CUSTOM_ELEMENTS_SCHEMA ],
  bootstrap: [ AppComponent ]
})
export class AppModule { }

app.component

import {Component} from '@angular/core';

@Component({
  selector: 'app',
  providers: [],
  template: `
    <custom-button [attr.title]="title"></custom-button>
  `
})
export class AppComponent {}

button.js

var toCustomElementV1 = require('nanocomponent-adapters/custom-element-v1')
var component = require('nanocomponent')
var html = require('bel')

// create new nanocomponent
var Button = component({
  render: function (data) {
    return html`
      <button>hello planet</button>
    `
  }
})
  
// register as custom element
// The second parameter corresponds to a string Array containing 
// the names of the attributes you'd like to observe and react to changes.
var CustomButton = toCustomElement(Button, ['title']);
customElements.define('custom-button', CustomButton);

To add a new component though, you would only have to add one new file, and
alter the app.component template

@alterx
Copy link
Collaborator Author

alterx commented Feb 23, 2017

You got it right. That's basically it.

@bcomnes bcomnes mentioned this pull request Aug 9, 2017
@alterx
Copy link
Collaborator Author

alterx commented Oct 18, 2017

@davidmarkclements I revisited these adapters over the weekend. The Custom Elements v0 one works but this version is deprecated. The v1 API adapter is intentionally written using the new ES2015 class syntax since it won't work with the old ES5 way (as stated in the first comment of this PR), there's a polyfill for that https://github.com/webcomponents/webcomponentsjs#custom-elements-es5-adapterjs but I'm not sure if we want to make it a hard requirement to use both this adapter and the polyfill, this one is your call.

I've also added a new Angular adapter.

(Update)

This repo can be used to test the changes https://github.com/alterx/ng-nanocomponent-adapters
Just clone it, npm link this branch and run with npm start

@yoshuawuyts
Copy link
Member

@bcomnes could you maybs take a look at this PR if you time? Thanks!

@alterx
Copy link
Collaborator Author

alterx commented Jan 19, 2018

Hey @yoshuawuyts, @bcomnes or someone else, is there something I can do to move this PR forward?

The last commit really changed how this works and addresses most of the concerns in the past comments. Would it help if I close this one and open a new one?

@toddself
Copy link

@alterx what is the best way I can test this? Is it possible to provide an automated test for it?

@alterx
Copy link
Collaborator Author

alterx commented Jan 19, 2018

Well, there's a repo you can clone and then npm link this branch.
I didn't provide any tests for the new implementation since:
a) The other adapters doesn't seem to have any tests
b) I didn't get any feedback in months, thought it was abandoned or something.

@alterx
Copy link
Collaborator Author

alterx commented Jan 19, 2018

@toddself, this is the repo: https://github.com/alterx/ng-nanocomponent-adapters
I added @angular/core as a peerDep because it felt kinda wrong to pass the whole instance to the adapter but I see that the react adapter does it this way. This one's your call. I can just revert this last commit and let it behave as the react adapter does.

@yoshuawuyts
Copy link
Member

I didn't provide any tests for the new implementation since:
a) The other adapters doesn't seem to have any tests
b) I didn't get any feedback in months, thought it was abandoned or something.

@alterx I'm so sorry we dropped the ball here! - yeah, tests would be great, but I reckon merging this first would probably be the right call ✨

@bcomnes
Copy link
Contributor

bcomnes commented Jan 19, 2018

@alterx yeah, super sorry, your pr fell off my radar and I never revisited it 😅

I'm going to give you commit access to this repo to help avoid that in the future.

@alterx
Copy link
Collaborator Author

alterx commented Jan 20, 2018

No problem guys, maintaining several OSS repos is no small feat 😄

I want to discuss something now that you guys are around:

When I created the Angular adapter I based my work in the React/Preact adapter and, as you can see here,

function toReact (Component, react) {
  assert.equal(typeof Component, 'function', 'nanocomponent-adapters/react: component should be type function')
  assert.equal(typeof react, 'object', 'nanocomponent-adapters/react: react should be type object')

the adapter receives an object which contains the React reference. Initially, I emulated this behavior in the Angular adapter but today I realized that we could just var react = require('react') at the top of the file without the need of sending in a reference. Maybe I'm being naive or I'm not familiar enough with this repo but it seems like this would reduce the api surface a bit (1 less parameter to send) and I think we can safely assume that an user installing these adapters is going to have the corresponding framework installed.

What do you think? @yoshuawuyts @bcomnes

(EDIT)
After looking at this it seems like requiring @angular/core inside the adapter instead of sending it in as a parameter breaks the adapter :( 😅
I'll take a look tomorrow to see if I can fix it, for the time being it's receiving the reference as a parameter.

@alterx
Copy link
Collaborator Author

alterx commented Jan 20, 2018

Ok, tested the adapter with several versions of angular:

  • 2.1.1
  • 4.0.0
  • 5.0.0

and simplified it a bit. Regarding my last comment the angular adapter still requires @angular/core as a parameter. If I try to require it inside the adapter angular won't recognize the generated component. There's a lot of configuration in angular apps (creating the decorators and so on) and passing the core as a reference is the easiest way to avoid manually doing this. So I guess you can scratch what I said 😅

@bcomnes
Copy link
Contributor

bcomnes commented Jan 29, 2018

@alterx I'll take a look at this tomorrow (out of time this evening). Sorry for the delay. I'm going to give you contributor access to help prevent delays in the future.

@bcomnes bcomnes merged commit 60625c4 into choojs:master Feb 12, 2018
@bcomnes
Copy link
Contributor

bcomnes commented Feb 12, 2018

Merging this to get it in. Going to review and make any changes / suggestions in subsequent PRs.

@bcomnes
Copy link
Contributor

bcomnes commented Feb 12, 2018

Updated the change-log, but I'm not sure what to version this. Major?

This brings up an important issue with modules like this. We should follow the https://github.com/maxogden/mississippi model and make these adapters individual repos and then perhaps re-collect them here so people can play around with them in one place and for the historical fact that this collection exists.

@bcomnes
Copy link
Contributor

bcomnes commented Feb 12, 2018

I'll create a new issue spec'ing out how to break this up individually. @alterx in the meantime, you have commit and release permissions. Feel free to cut the release as you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants