choojs / nanocomponent-adapters

🔌 - Convert a nanocomponent to a component for your favourite API or library (web components, (p)react, angular)
MIT License
96 stars 17 forks source link

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

Closed alterx closed 6 years ago

alterx commented 7 years ago

I added a couple of things here:

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.

davidmarkclements commented 7 years ago

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 commented 7 years ago

You got it right. That's basically it.

alterx commented 6 years ago

@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 commented 6 years ago

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

alterx commented 6 years ago

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 commented 6 years ago

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

alterx commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 :sparkles:

bcomnes commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

Ok, tested the adapter with several versions of angular:

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 commented 6 years ago

@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 commented 6 years ago

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

bcomnes commented 6 years ago

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 commented 6 years ago

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.