eiriklv / react-masonry-component

A React.js component for using @desandro's Masonry
MIT License
1.44k stars 145 forks source link

TypeScript definition d.ts file? #42

Closed empz closed 8 years ago

empz commented 8 years ago

Would you consider including a TypeScript definition file in the package?

http://www.typescriptlang.org/docs/handbook/typings-for-npm-packages.html

afram commented 8 years ago

@emzero Yeah no issues with that

afram commented 8 years ago

@emzero do you feel like contributing one? 😛

empz commented 8 years ago

@afram I'm actually pretty new to TypeScript and definitions so I did my bare minimum to make it work.

Here's what I got. I don't think a full PR is worth it, so here you go. Anyone is welcome to improve it.

declare module "react-masonry-component" {
  import React = __React;

  interface MasonryPropTypes {
    disableImagesLoaded: boolean;
    options: Object;
    className: string;
    elementType: string
  }

  export var Masonry: React.Component<MasonryPropTypes, void>;
}
afram commented 8 years ago

@emzero No worries.

I don't use TypeScript, please let me know how this beta version works for you. If it's all good I can release the next version.

npm i react-masonry-component@4.1.1-beta1 -S

wjramos commented 8 years ago

So, I am actually curious -- why TypeScript over React PropTypes?

MasonryComponent.propTypes = {
  disableImagesLoaded: PropTypes.bool,
  onImagesLoaded:      PropTypes.func,
  options:             PropTypes.object
};

See my fork for example:

https://github.com/wjramos/react-masonry-component/blob/masonry/components/MasonryComponent.js

empz commented 8 years ago

@wjramos With TypeScript there's no need to use React PropTypes since a strongly-typed component using an interface to describe the props provides the same type checking at compile time.

Related

empz commented 8 years ago

@afram That doesn't work because it depends on React typings being installed as a global module and might not be always the case. I'm also getting another error from Typescript.

TypeScript 2.0 is about to be released and it will drastically improve the way d.ts files are distributed for npm packages. So I would suggest that we just wait for it to include a proper d.ts file.

In the meantime, you could include a section in the readme saying something like...

If you are using TypeScript, include the following code in your custom definition file.

declare module "react-masonry-component" {
  import React = __React;

  interface MasonryPropTypes {
    disableImagesLoaded: boolean;
    options: Object;
    className: string;
    elementType: string
  }

  export var Masonry: React.Component<MasonryPropTypes, void>;
}
wjramos commented 8 years ago

@emzero wouldn't it be adding an otherwise unnecessary dependency?

empz commented 8 years ago

@wjramos What do you mean?

EDIT: I think I understand what you're saying now.

I opened this issue to ask the author to include TypeScript definitions. This does not mean the library is going to depend on TypeScript. It's just a file that's needed if the library is going to be used in a TypeScript environment.

The original Javascript can still make use of PropTypes while the Typescript definition file uses an interface to define the props.

afram commented 8 years ago

@emzero I've updated the typings to import React directly. Can you please let me know if this works for you?

This was a quick "fix" based on an hour of reading, not sure if it's going to work.

Either way, you've motivated me to start taking TypeScript a bit more seriously.

react-masonry-component@4.1.1-beta3

empz commented 8 years ago

The TypeScript compiler is complaining.

ERROR in node_modules\react-masonry-component\typings.d.ts (5,15):
error TS2665: Module augmentation cannot introduce new names in the top level scope.

ERROR in node_modules\react-masonry-component\typings.d.ts (14,16):
error TS2665: Module augmentation cannot introduce new names in the top level scope.

I actually have no idea how to make use of another external lib's typings. I've posted a question on SO but no answers yet.

As I said, TypeScript 2.0 will use npm to manage typings so it should provide better dependency management.

afram commented 8 years ago

@emzero I think I may have found a possible solution, can you please try this version? Appreciate your help in getting it out there :-)

react-masonry-component@4.1.1-beta4

empz commented 8 years ago

@afram

I'm using typescript@next which is the 2.0 preview version for the next TS version that's just around the corner. The following declaration file works great with this version. I wouldn't really bother to make it work with 1.8 today.

import { Component } from "react";

declare module "react-masonry-component" {

  export interface MasonryOptions {
    columnWidth?: number;
    itemSelector?: string;
    gutter?: number;
    percentPosition?: boolean;
    stamp?: string;
    fitWidth?: boolean;
    originLeft?: boolean;
    originTop?: boolean;
    containerStyle?: Object;
    transitionDuration?: string;
    resize?: boolean;
    initLayout?: boolean;
  }

  interface MasonryPropTypes {
    disableImagesLoaded?: boolean;
    updateOnEachImageLoad?: boolean;
    onImagesLoaded?: (instance: any) => void;
    options?: MasonryOptions;
    className?: string;
    elementType?: string;
  }

  export default class Masonry extends Component<MasonryPropTypes, void> { }
}

I've also added an interface for the options.

afram commented 8 years ago

Great thanks, this has been merged into v4.2.0

empz commented 8 years ago

@afram Could you add a line to the interface MasonryPropTypes so we can add styles to it?

interface MasonryPropTypes {
    disableImagesLoaded?: boolean;
    updateOnEachImageLoad?: boolean;
    onImagesLoaded?: (instance: any) => void;
    options?: MasonryOptions;
    className?: string;
    elementType?: string;
    style?: Object: // ADDED
  }
afram commented 8 years ago

done v4.2.1