Skip to content
This repository has been archived by the owner on Jan 11, 2018. It is now read-only.

refactor: add latest bore and allow ts declaration generation #157

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,8 @@ declare module '*.css';
declare module '*.json';

// Skate
declare module 'skatejs-web-components';
declare module 'core-js';

// Bore
declare module 'bore' {

interface BoreNode extends HTMLElement {
}
interface WrappedNode extends Wrapper {
node: BoreNode,
}
type Query<T> = string | JSX.Element | T | ((node: BoreNode) => boolean) | Object;

interface Wrapper {
readonly shadowRoot: ShadowRoot;
all<T extends HTMLElement>(query: Query<T>): WrappedNode[],
one<T extends HTMLElement>(query: Query<T>): WrappedNode,
has<T extends HTMLElement>(query: Query<T>): boolean,

wait(callback?: (wrapper: WrappedNode) => any): Promise<WrappedNode>,
waitFor(callback: (wrapper: WrappedNode) => boolean, options?: { delay?: number }): Promise<WrappedNode>,
}
export function mount(htmlOrNode: JSX.Element | JSX.Element[] | string): WrappedNode;
export function h(name: string, attrsOrProps?: Object, ...children: any[]): JSX.Element | JSX.Element[];

}


// Custom Elements
declare const customElements: CustomElementRegistry;
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"@types/mocha": "^2.2.37",
"@types/sinon-chai": "^2.7.27",
"awesome-typescript-loader": "^3.0.3",
"bore": "^2.0.0",
"bore": "^2.0.1",
"commitizen": "^2.9.5",
"concurrently": "^3.1.0",
"css-loader": "^0.26.1",
Expand Down
19 changes: 11 additions & 8 deletions packages/_helpers/bore-h.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,36 @@
/**
* @deprecated
*/
export function h( name: any, attrs: { [ key: string ]: any }, ...chren: any[] ) {
const node = typeof name === 'function' ? handleFunction( name ) : document.createElement( name );
Object.keys( attrs || [] ).forEach(( attrName: string ) =>
shouldBeAttr( attrName, attrs[ attrName ] )
? ( attr( node, attrName, attrs[ attrName ] ) )
: ( node[ attrName ] = attrs[ attrName ] )
);
chren.forEach( (child) => node.appendChild( child instanceof Node ? child : document.createTextNode( child ) ) );
chren.forEach(( child ) => node.appendChild( child instanceof Node ? child : document.createTextNode( child ) ) );
return node;
}

function startsWith (key: string, val: string) {
return key.indexOf(val) === 0;
function startsWith( key: string, val: string ) {
return key.indexOf( val ) === 0;
}
function shouldBeAttr (key: string, val: string) {
return startsWith(key, 'aria-') || startsWith(key, 'data-') || isAttribute(key);
function shouldBeAttr( key: string, val: string ) {
return startsWith( key, 'aria-' ) || startsWith( key, 'data-' ) || isAttribute( key );
}
function isAttribute( key: string ) {
return key === 'attributes';
}

function handleFunction (Fn: any) {
function handleFunction( Fn: any ) {
return Fn.prototype instanceof HTMLElement ? new Fn() : Fn();
}

// JQuery like attr
function attr( node: any, attrName: string, attrValue: any ): void {
if ( isAttribute(attrName) ) {
if ( isAttribute( attrName ) ) {
Object.keys( attrValue ).forEach(( key ) => {
node.setAttribute( key, attrValue[key] );
node.setAttribute( key, attrValue[ key ] );
});
return;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/_helpers/colorTypes.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const ColorTypes = {
export const ColorTypes = {
brand: 'brand',
info: 'info',
warning: 'warning',
Expand Down
3 changes: 3 additions & 0 deletions packages/_helpers/definitions/events.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export type ClickEvent = typeof HTMLElement.prototype.onclick;
export type FocusEvent = typeof HTMLElement.prototype.onfocus;
export type BlurEvent = typeof HTMLElement.prototype.onblur;
4 changes: 4 additions & 0 deletions packages/_helpers/definitions/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// @TODO this should maybe be a part of Skate
export type IntrinsicCustomElement<P> = P & Partial<HTMLElement>;
// @TODO this should maybe be a part of Bore
export type IntrinsicBoreElement<A, E> = { attrs?: A } & { events?: E };
4 changes: 4 additions & 0 deletions packages/_helpers/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import * as GenericEvents from './definitions/events';
import * as GenericTypes from './definitions/types';

export { GenericEvents, GenericTypes };
export * from './colorTypes';
export * from './css';
export * from './sizes';
Expand Down
2 changes: 1 addition & 1 deletion packages/_helpers/sizes.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const Sizes = {
export const Sizes = {
xsmall: 'xsmall',
small: 'small',
medium: 'medium',
Expand Down
67 changes: 43 additions & 24 deletions packages/button/Button.test.tsx
Original file line number Diff line number Diff line change
@@ -1,45 +1,64 @@
import * as expect from 'expect';
import { h } from '../_helpers';
import { mount } from 'bore';
import { h, mount } from 'bore';
import { emit } from 'skatejs';

import { Button } from './index';

describe( Button.is, () => {

describe( `Custom element`, () => {
it( `should be registered`, () => {
expect( customElements.get( Button.is ) ).toBe( Button );
});
it( `should render via JSX IntrinsicElement`, () => {
return mount( <bl-button>Hello</bl-button> ).wait( (element) => {
expect( element.node.localName ).toBe( Button.is );
describe( `Custom element`, () => {

it( `should be registered`, () => {
expect( customElements.get( Button.is ) ).toBe( Button );
});
});
it( `should render`, () => {
return mount( <Button /> ).wait(( element ) => {
expect( element.has( '.c-button' ) ).toBe( true );

it( `should render via JSX IntrinsicElement`, () => {
return mount( <bl-button>Hello</bl-button> ).wait(( element ) => {
expect( element.node.localName ).toBe( Button.is );
});
});

it( `should render`, () => {
return mount( <Button /> ).wait(( element ) => {
Copy link
Collaborator

@elmariofredo elmariofredo Feb 3, 2017

Choose a reason for hiding this comment

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

shouldn't we use <bl-button> here? I know that it's stated here https://github.com/wc-catalogue/blaze-elements/blob/master/docs/styleguides/TESTING.md#example--button- but I did forgot why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

shouldn't we use here?

use what?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ups sorry forgot back ticks around the tag

Copy link
Collaborator

Choose a reason for hiding this comment

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

👋 still waiting for an answear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't remember either ¯_(ツ)_/¯

maybe few suggestions about your review attitude:

  • if you intend to stop merging things because some unrelated unanswered questions, that's very unfortunate for velocity and for everyone that's participating on this project
  • we can change it ofc in another PR which should refactor other tests that have the same pattern and update the docs accordingly

Copy link
Collaborator

Choose a reason for hiding this comment

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

then please fix it to <bl-button>

thanks

p.s. this tone of suggestion is really unfortunate, point of PR is answering questions. PR comments are not the right place for discussing attitudes and any PR unrelated topic. Please contact me directly if you want to discuss this further. Thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you pls unblock merging of this PR. You are blocking rest of the team. thx

expect( element.has( '.c-button' ) ).toBe( true );
});
});
});
});

describe( `API`, () => {
describe( `API`, () => {

describe( `[color]`, () => {
describe( `[color]`, () => {

it( `should render no color class on button by default`, () => {
return mount(<bl-button attributes={{color: 'brand'} as any}>huhuh</bl-button>).wait(( element ) => {
expect( element.one( 'button' ).node.className ).toContain( 'c-button' );
it( `should render no color class on button by default`, () => {
return mount( <bl-button attrs={{ color: 'brand' }}>huhuh</bl-button> ).wait(( element ) => {
expect( element.one( 'button' ).node.className ).toContain( 'c-button' );
});
});

it( `should render color class on button`, () => {
return mount( <bl-button attrs={{ color: 'info' }}>hello</bl-button> ).wait(( element ) => {
expect( element.has( '.c-button--info' ) ).toBe( true );
});
});

});

it( `should render color class on button`, () => {
return mount( <bl-button attributes={ { color: 'info' } as any }>hello</bl-button> ).wait(( element ) => {
expect( element.has( '.c-button--info' ) ).toBe( true );
describe( `events`, () => {

it( `should handle click`, () => {

let clickTriggered = false;
const handleClick = ( e: MouseEvent ) => { clickTriggered = true; };

return mount( <bl-button events={{ click: handleClick }}>Click me</bl-button> )
.wait(( element ) => {
emit( element.node, 'click' );
expect( clickTriggered ).toBe( true );
});
});

});

});

});

});
30 changes: 20 additions & 10 deletions packages/button/Button.tsx
Original file line number Diff line number Diff line change
@@ -1,33 +1,43 @@
import { h, Component, prop } from 'skatejs';
import { h, Component, prop, ComponentProps } from 'skatejs';

import { ColorType, cssClassForColorType, css } from '../_helpers';
import {
ColorType,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking - what about change order of imports to first helpers, then types? like:
import { cssClassForColorType, css, ColorType, IntrinsicCustomElement, IntrinsicBoreElement, ClickEvent } from '../_helpers'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm thats too much nit picking IMHO. But I see your point. We are importing a lot of stuff from helpers but that's what is it for.

Important is to always order imports in order

  • 3rd party
  • local ( non package related ) - this should go away when we have proper build
  • package internal relative imports

Maybe we can extract this definitions to root /packages/definitions.ts

Also that _helpers folder is wrong, it should be separate package named core or base

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe next time 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cheers mate 🍺

cssClassForColorType,
css,
GenericTypes,
GenericEvents
} from '../_helpers';

import styles from './Button.scss';

// public
type ButtonProps = Props & EventProps;
type Props = {
export type ButtonProps = Props & EventHandlers;

export type Props = {
disabled?: boolean,
block?: boolean,
close?: boolean,
ghost?: boolean,
color?: ColorType,
};
type EventProps = {
onClick?: typeof HTMLElement.prototype.onclick,

export type Events = {
click?: GenericEvents.ClickEvent,
};
export type EventHandlers = {
onClick?: GenericEvents.ClickEvent,
};

declare global {
namespace JSX {
interface IntrinsicElements {
'bl-button': ButtonProps & Partial<HTMLElement>,
'bl-button': GenericTypes.IntrinsicCustomElement<ButtonProps> & GenericTypes.IntrinsicBoreElement<Props, Events>
}
}
}

export class Button extends Component<ButtonProps> {
static get is() { return 'bl-button'; }
static get props() {
static get props(): ComponentProps<Button, Props> {
return {
disabled: prop.boolean( {
attribute: true
Expand All @@ -47,7 +57,7 @@ export class Button extends Component<ButtonProps> {
source: true
}
}),
color: prop.string( {
color: prop.string<Button, ColorType>( {
attribute: {
source: true
}
Expand Down
8 changes: 4 additions & 4 deletions packages/button/index.demo.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { h, Component, prop, props } from 'skatejs';
import { h, Component, prop, props, ComponentProps } from 'skatejs';
import { Button } from './index';

type DemoProps = { logger: string[] };
export type DemoProps = { logger: string[] };
export class Demo extends Component<DemoProps> {
static get is() { return 'bl-button-demo'; }

static get props() {
static get props(): ComponentProps<Demo, DemoProps> {
return {
logger: prop.array()
logger: prop.array<Demo, string>()
};
}

Expand Down
1 change: 1 addition & 0 deletions packages/button/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "1.2.0",
"main": "dist/main.js",
"license": "MIT",
"typings": "definitions/index.d.ts",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is up to discussion, because current build is non existent till #153

Copy link
Collaborator

Choose a reason for hiding this comment

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

well template should reflect desired state for each package, also it doesn't have to do anything with our current build if definitions are not working it shouldn't be here as well.

Simply I want to have package.json to be auto generated to some degree so we can easily change tooling for all packages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My comment was about the path of typings within package.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

as you know there is build process from very beggining but its building test files instead of production bundles to dist folder, not sure how this is related to #153 if it is related somehow then you should wait before #153 is closed and even then add it to the template as well

"scripts": {
"start": "../../node_modules/.bin/webpack-dev-server --config ../../webpack.config.js --env.dev --hot --host 0.0.0.0 --env.element=button",
"test": "npm run build:test && ../../node_modules/.bin/wct -l firefox",
Expand Down
4 changes: 4 additions & 0 deletions packages/button/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as 👆

"declaration": true,
"declarationDir": "definitions"
},
"include": [
"../../global.d.ts",
"*.ts",
Expand Down
1 change: 1 addition & 0 deletions toolbelt/templates/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "bl-generic",
"version": "1.0.0",
"main": "dist/main.js",
"typings": "definitions/index.d.ts",
"license": "MIT",
"scripts": {
"start": "../../node_modules/.bin/webpack-dev-server --config ../../webpack.config.js --env.dev --hot --host 0.0.0.0 --env.element=packageName",
Expand Down
5 changes: 5 additions & 0 deletions toolbelt/templates/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"declaration": true,
"declarationDir": "definitions"
},
"include": [
"../../global.d.ts",
"*.ts",
"*.tsx"
]

}
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"noUnusedLocals": true,
"noEmitHelpers": true,
"importHelpers": true,
"stripInternal": true,
"moduleResolution": "node",
"outDir": "ts-output",
"rootDir": "./",
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ boom@2.x.x:
dependencies:
hoek "2.x.x"

bore@^2.0.0:
bore@^2.0.1:
version "2.0.1"
resolved "https://registry.yarnpkg.com/bore/-/bore-2.0.1.tgz#25b561fced51611e3af195a4635b8919b0de9c54"
dependencies:
Expand Down