Skip to content
This repository was archived by the owner on Dec 19, 2017. It is now read-only.

Typescript type declarations #142

Closed
wants to merge 7 commits into from
Closed

Typescript type declarations #142

wants to merge 7 commits into from

Conversation

matt-hensley
Copy link
Collaborator

These likely need another revision, Typescript inference fails on some of the middleware types. This does not break compilation, just type safety.

ko-component-router.d.ts contains the Typescript declarations
package.json has a new field (types) to allow Typescript to discover the declarations

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.585% when pulling 236d047 on barake:types into a57f348 on Profiscience:master.

@caseyWebb
Copy link
Contributor

@barake I don't know much about TS, so I'm going to leave this to your discretion as to when it's ready. Just say when.

@matt-hensley
Copy link
Collaborator Author

Will update with setConfig/useRoutes from #143

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.68% when pulling 53de915 on barake:types into a57f348 on Profiscience:master.

@matt-hensley
Copy link
Collaborator Author

@Ixonal care to review?

@Ixonal
Copy link

Ixonal commented Jan 11, 2017

@barake Just noticed one thing, though I only did a quick once-over. The only issue I had was that the main export isn't a constructor. It only came up because then I can't use it in my IOC container. I've been using a (less complete) definition file that sets the main export using an interface to accommodate that.

interface KoComponentRouterContext {
  pathname: string;
  router: KoComponentRouterInstance;
}

interface KoComponentRouterMiddleware {
  (ctx?: KoComponentRouterContext, done?: () => void): void;
}

interface KoComponentRouterInstance {
  ctx: KoComponentRouterContext;
  isNavigating: KnockoutObservable<boolean>;
  isRoot: boolean;
  update(path: string, push: boolean): void;
}

interface KoComponentRouter {
  new (): KoComponentRouterInstance;
  get(index: number): KoComponentRouterInstance;
  use(middleware: Function): void;
  usePlugin(plugin: Function): void;
}

declare module "ko-component-router" {
  let Router: KoComponentRouter;
  export = Router;
}

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.68% when pulling 9e53237 on barake:types into d28b2a8 on Profiscience:master.

@matt-hensley
Copy link
Collaborator Author

@Ixonal check out this update, should allow you to instantiate the exported type now

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.68% when pulling 7666e35 on barake:types into d28b2a8 on Profiscience:master.

@Ixonal
Copy link

Ixonal commented Jan 11, 2017

@barake Copied over the new definition file and Typescript now complains about importing a non-module entity.

@jonathaneckman
Copy link

@caseyWebb I need these typings. Is this ready to be merged?

@matt-hensley
Copy link
Collaborator Author

@SystemAcc0unt I haven't asked for these to be merged as @lxonal had issues with them. I'm using the definitions daily with TS 2.x just fine (we maintain internal repos for custom types).

This PR needs to be updated with some of the new features @caseyWebb has added recently.

@Ixonal
Copy link

Ixonal commented Feb 21, 2017

I've been using my custom definition, waiting to see if this would get updated to fix the last problem I ran into. I'm using TS 2.1.4 right now, btw.

@matt-hensley
Copy link
Collaborator Author

Can you share a gist of how you import and your current type definition? I'll try and get this figured out for everybody.

@Ixonal
Copy link

Ixonal commented Feb 22, 2017

I import it like this:

import * as Router from "ko-component-router";

any other syntax, and Router is undefined for me.

I'm still using the definitions I posted earlier. The main difference, from what I can tell, is what I'm exporting in the module. In particular, that KoComponentRouter interface having the constructor definition is what allows me to pass it as a constructor. That being said, I ended up abstracting away the router itself, so I don't really need to feed it to the IOC container anymore (though, since it IS a constructor, it probably should have that anyways).

@matt-hensley
Copy link
Collaborator Author

The built library is a commonjs module that basically does this:
module.exports = Router;

TypeScript uses these syntax for commonjs imports:
import Router = require('ko-component-router');

@caseyWebb
Copy link
Contributor

Deferring to the community (who knows far more about TS than I). Y'all let me know when it's ready and I'll merge.

@Ixonal
Copy link

Ixonal commented Feb 22, 2017

"import something = require('something')" is using their older (typescript specific) syntax for imports. The "import * as something from 'something'" syntax does effectively the same thing, but is preferred since it is actual javascript syntax.

Edit: actually, it might also be something pertaining to Webpack that's making that work. Regardless, the import itself IS working.

@jonathaneckman
Copy link

I import it like this:

import * as Router from "ko-component-router";

any other syntax, and Router is undefined for me.

Without a definition file, I am unable to import because there is no module named ko-component-router. Until we can merge this, is my only option is to create my own d.ts or do you know a more elegant solution? I may be getting false negatives because ko.router is never defined and I have no way to import any modules from the library. I am using webpack 2 and TypeScript 2.2. It seemed to work fine in ko-component-router v3, but Im still investigating to determine if it broke from a change in v4, or a change in my apps setup.

@Ixonal
Copy link

Ixonal commented Feb 22, 2017

@jonathaneckman The custom definitions are still how I'm solving the issue until an official definition is included. I suppose you could force a simple solution to bypass the type checking

declare module "ko-component-router" {
  let Router: any;
  export = Router;
}

But that's really just saying that the module exists and export is 'any' type, which isn't very helpful...

I haven't tried to access ko.router myself, so I can't really say if anything happened there.

@Ixonal
Copy link

Ixonal commented Feb 22, 2017

@barake I reworked your definitions a little to make them work with my setup. Does it work with yours, or is there a more fundamental difference between our setups that's causing an issue?

interface KoComponentRouterRoute {
  component: string;
}

type CallbackResult = void | false | Promise<boolean>;
type DoneCallback = (boolean) => void;

type KoComponentRouterPromiseMiddleware = (ctx: KoComponentRouterContext) => CallbackResult;
type KoComponentRouterCallbackMiddleware = (ctx: KoComponentRouterContext, done: DoneCallback) => void;
type KoComponentRouterLifecycleMiddleware = (ctx: KoComponentRouterContext) => {
  beforeRender: () => CallbackResult;
  afterRender: () => CallbackResult;
  beforeDispose: () => CallbackResult;
  afterDispose: () => CallbackResult;
};
type KoComponentRouterMiddleware = KoComponentRouterPromiseMiddleware
  | KoComponentRouterCallbackMiddleware
  | KoComponentRouterLifecycleMiddleware;

interface KoComponentRouterContext {
  $child: KoComponentRouterContext;
  $parent: KoComponentRouterContext;
  addBeforeNavigateCallback(cb: () => CallbackResult);
  addBeforeNavigateCallback(cb: (done: DoneCallback) => void);
  canonicalPath: string;
  element: HTMLElement;
  fullPath: string;
  params: any;
  path: string;
  pathname: string;
  route: KoComponentRouterRoute;
  router: KoComponentRouterInstance;
}

//represents an instance of a router
interface KoComponentRouterInstance {
  $child: KoComponentRouterInstance;
  $parent: KoComponentRouterInstance;
  base: string;
  ctx: KoComponentRouterContext;
  isRoot: boolean;
  isNavigating: KnockoutObservable<boolean>;

  getPathFromLocation(): string;
  resolvePath(path): any;
  update(path: string, push?: boolean): Promise<boolean>;
}

interface KoComponentRouterRoutes {
  [path: string]: string
  | KoComponentRouterRoutes
  | Array<string | KoComponentRouterPromiseMiddleware | KoComponentRouterLifecycleMiddleware>;
}

interface KoComponentRouterConfig {
  base?: string;
  hashbang?: boolean;
}

//represents the static interface of the router
interface KoComponentRouterStatic {
  config: KoComponentRouterConfig;
  middleware: KoComponentRouterMiddleware[];
  plugins: any[];
  routes: KoComponentRouterRoutes;
  head: KoComponentRouterInstance;
  tail: KoComponentRouterInstance;

  canonicalizePath(path: string): string;
  get(index: number): KoComponentRouterInstance;
  getPath(url: string): string;
  parseUrl(url: string): { hash: string, pathname: string, search: string };
  sameOrigin(href: string): boolean;
  setConfig(config: KoComponentRouterConfig): void;
  update(path: string, push?: boolean): Promise<boolean>;
  update(path: string, options: { push?: boolean; force?: boolean; with?: any }): Promise<boolean>;
  use(...middleware: KoComponentRouterMiddleware[]): void;
  usePlugin(...fns): void;
  useRoutes(routes: KoComponentRouterRoutes): void;
}

//represents the type of the router
interface KoComponentRouter extends KoComponentRouterStatic {
  new (...args): KoComponentRouterInstance;
}

declare module 'ko-component-router' {
  let Router: KoComponentRouter;
  export = Router;
}

@matt-hensley
Copy link
Collaborator Author

Both definitions work fine. I was attempting to follow TS best practices, as I understand them.

ko-component-router is exporting the Router class by default:

export default Router

So we should be able to make a TS definition that allows:

import Router from 'ko-component-router'

Except the library is being built in to a commonjs module - not an ES6 module. According to ahejlsberg:

A module that uses export = to export a non-module entity in place of the module itself must be imported using the existing import x = require("foo") syntax as is the case today.

So to import the default value of a commonjs module, you do:

import Router = require('ko-component-router')

Does the ES6 import/export spec allow for this?

import * as Router from 'ko-component-router'
var router = new Router()

Some additional reading on TS imports:

Sorry for the verbosity. Wanted to get my whole thought process down.

@Ixonal
Copy link

Ixonal commented Feb 22, 2017

Interesting. I hadn't seen that the router being exported as default... Is that a problem with the build system, then? I would've preferred to do the default import syntax instead, but since it was returning undefined, that was kind of out.

From what I'm reading, both the 'import something = require' and 'import * as something' syntaxes do the same thing. 'import * as something' appears to be slightly misrepresenting the ES6 spec, as it's supposed to be used for namespaces rather than, say, classes, but 'import something = require' is considered legacy, so I'd be a little iffy on using it for new stuff.

This issue looks to be relevant...
microsoft/TypeScript#5565
After reading that, I performed an experiment. I added

Router$1["default"] = Router$1;

just before the last return in the built javascript file for the lib and was able to get a default import for it working.

@matt-hensley
Copy link
Collaborator Author

@caseyWebb unfortunately not, it's up the bundler to implement. TS does not examine the package.json for a module and rewrite the import.

@Ixonal
The problem with import * as foo ... is that foo wouldn't ever be a constructor in ES6, near as I can tell by playing with TS and reading the import/export spec.

You are correct regarding adjusting the export to be default.

import Router from 'ko-component-router'
let instance = new Router()

gets transpiled to

var ko_component_router_1 = require("ko-component-router");
var instance = new ko_component_router_1["default"]();

Babel 5 used to do this IF you only had a single, default export.

Object.defineProperty(exports, "__esModule", { value: true });
exports.default = 'foo';
module.exports = exports['default'];

Modules are a mess :(

@Ixonal
Copy link

Ixonal commented Feb 23, 2017

@barake Yeah, looks like in ES6, import * as ... is supposed to be for namespaces (or rather, plain javascript objects). Looks like TS slightly altered that logic to be able to use it with export = ... legacy modules, which can be anything (including functions).

What does babel emit for default imports/exports now? Of course, this project is compiled with buble, so I guess it also matters how that lib handles default imports/exports.

Edit: Actually, it looks like rollup is handling modules for this project

@caseyWebb
Copy link
Contributor

caseyWebb commented Feb 24, 2017

The build process is funky b/c I was trying to minimize the file size. It's since grown, and I rewrote the generator since it is expensive to transpile either way, so I'm not sure there is much to gain.

I'm not opposed to changing the build process or source, so long as we still export a browser/cjs bundle in the same location.

I've actually tossed around the idea of rewriting the actual router in TS, but I don't have the time to commit to learning it at the moment. With that being said, I'm willing to work with anyone 100% who is willing to take a lead in converting it to TS.

@caseyWebb
Copy link
Contributor

afaik, the TS compiler is as good as babel now and can be used to build and export an ES5 commonjs/browser bundle. Would that pretty much allow you to add the typings in the source with minimal changes otherwise?

@Ixonal
Copy link

Ixonal commented Feb 24, 2017

@caseyWebb There are some minor differences in the output, but they should be largely on par. You can set what module system to use (which, if ES5, will default to common js). You can also set "declaration" to true to specify that you want it to generate the .d.ts files. Compiler options are listed here. As is, you should be able to compile exactly what you already have with typescript successfully; everything will just be considered "any" type.

@caseyWebb caseyWebb added this to the v5 milestone Mar 6, 2017
@matt-hensley
Copy link
Collaborator Author

There is real work being done to move the router to TypeScript.

In the interim, copy one of the proposed definition out of the PR comments and use locally.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants