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

Couple of issues with new typings in @horizon/client #834

Closed
samdesota opened this issue Sep 20, 2016 · 15 comments
Closed

Couple of issues with new typings in @horizon/client #834

samdesota opened this issue Sep 20, 2016 · 15 comments

Comments

@samdesota
Copy link

Hey All,

Thanks for adding typings to the horizon client! Really helps with my workflow.

I noticed a couple of issues, though. First, fetch() and watch() are incorrectly typed to return TermBase when the should return a observable. Like this

        watch (options?: { rawChanges: boolean }): Observable<any>;
        fetch (): Observable<any>;

I imagine this is a mistake, but possibly it's an API change?

Also, if I want to create a function that explicitly accepts a HorizonInstance, like (hz: HorizonInstance) => TermBase you need to add these 2 lines to your typings so I can access the types.

export type HorizonInstance = hz.HorizonInstance;
export type TermBase = hz.TermBase;
export type Collection = hz.Collection;

Thanks!

@marshall007
Copy link
Contributor

@mrapogee thanks, .fetch() and .watch() returning TermBase is my mistake. I threw the typings together really quick and didn't spend much time testing. I'll get another PR up with all the changes you mentioned. Let me know if you stumble across anything else that's missing/incorrect in the public API.

@samdesota
Copy link
Author

Hey,

Couple more things.

  • Some methods were missing from HorizonInstance.
hasAuthToken (): Boolean;
clearAuthTokens (): void;
authEndpoint (authProvider: string): Observable<string>;
  • You need to add this line to your src index.js
Horizon.default = Horizon

This is because of the way typescript resolves imports. Typescript basically compiles import Horizon from '@horizon/client' to require('@horizon/client').default. But,since you are using export = Horizon in your index, .default will be undefined.

@marshall007
Copy link
Contributor

marshall007 commented Sep 21, 2016

This is because of the way typescript resolves imports.

@mrapogee is this still true in the latest versions of Typescript? In the project I was working on (using angular-cli) the import Horizon from '@horizon/client' compiled and ran fine for me.

@marshall007
Copy link
Contributor

I think microsoft/TypeScript#5285 may be related (fixed in v1.8).

@marshall007
Copy link
Contributor

@mrapogee also, .clearAuthTokens() is only defined statically. I added the other two missing methods though. Just let me know on the TS module resolution thing and we'll be good to go.

deontologician pushed a commit that referenced this issue Sep 21, 2016
* fix return types for `.fetch()` and `.watch()`, export additional types

Fixes #834

* add `.hasAuthToken()` and `.authEndpoint()`
@samdesota
Copy link
Author

samdesota commented Sep 21, 2016

I'm using typescript@2.1.0-dev.20160826 and it resolves to undefined without it. I think it works if you use the system module system with typescript, but many people aren't going to want to change their module system for a package, and for various reasons often can't.

@marshall007
Copy link
Contributor

marshall007 commented Sep 21, 2016

@mrapogee if you're not using system module for code generation, then shouldn't you be using const Horizon = require('@horizon/client') rather than the import syntax anyway? I'm not sure you're supposed to expect that to work otherwise. I could be wrong though, can you point to any docs?

[edit]: also, did you try using the --allowSyntheticDefaultImports compiler option? This seems to be what that flag is for.

@samdesota
Copy link
Author

samdesota commented Sep 21, 2016

Checkout https://www.typescriptlang.org/docs/handbook/modules.html

Doing const Horizon = require('@horizon/client') won't import types. It just sets Horizon as type any

But, I can do import Horizon = require('@horizon/client') for a module that exports a function directly, but that means you need to change your index.d.ts to export = Horizon, as microsoft/TypeScript#5285 shows. If you do this, you can't export types from the typings file, you can only export one thing (The horizon initializer).

@abecks
Copy link

abecks commented Sep 30, 2016

Is it possible to include the Horizon Client with type definitions right now? I'm still pretty new to Horizon and TypeScript, but shouldn't this be close?

import * as Horizon from '@horizon/client'; // [ts] Cannot find module '@horizon/client'

Also it appears the NPM version doesn't have the definition file yet, so I copied it into my project manually. Can't get it to recognize the name of the module for the life of me.

@samdesota
Copy link
Author

It's working with this tsconfig.json. You probably just need moduleResolution set to node and you can add "typings": "index.d.ts" to the @horizon/client package.json.

{
  "compilerOptions": {
    "moduleResolution": "node",
    "module": "commonjs",
    "noImplicitAny": true,
    "removeComments": true,
    "experimentalDecorators": true,
    "strictNullChecks": true,
    "noImplicitThis": true,
    "typeRoots": [
      "./node_modules/@types"
    ],
    "target": "es6"
  }
}

You can import it like import Horizon from '@horizon/client', but you'll need to add Horizon.default = Horizon to the end of @horizon/client/lib/index.js. That's my current setup and it's working great.

@abecks
Copy link

abecks commented Sep 30, 2016

Thanks for the help. I still can't seem to get it to recognize the import path. I added the moduleResolution parameter to my tsconfig.json, which now looks like:

{
    "compilerOptions": {
        "outDir": "./dist/",
        "sourceMap": true,
        "noImplicitAny": true,
        "moduleResolution": "node",
        "module": "commonjs",
        "target": "es5",
        "jsx": "react"
    }
}

and my Horizon.tsx file:

import Horizon from '@horizon/client'; // [ts] Cannot find module '@horizon/client'.

const horizon = new Horizon();
horizon.onReady(function () {
  document.querySelector('h1').innerHTML = 'Horizon works!'
});
horizon.connect();

I do have @horizon/client installed in node_modules, and I modified the package.json and lib/index.js accordingly.

Still getting the error Cannot find module '@horizon/client'. Any ideas?

@marshall007
Copy link
Contributor

@abecks the typings I've added have been merged into next but not released. If you manually npm install @horizon/client@next they should get picked up automatically.

@abecks
Copy link

abecks commented Sep 30, 2016

Nevermind, I was missing the index.d.ts file still. I copied it over from the master here and now the import is recognized.

Thanks @marshall007 I'll switch to next now.

@abecks
Copy link

abecks commented Sep 30, 2016

Hey @marshall007, getting the following npm error when trying to install the next branch.

npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install" "@horizon/client@next"
npm ERR! node v4.6.0
npm ERR! npm  v3.10.8
npm ERR! code ETARGET

npm ERR! notarget No compatible version found: @horizon/client@next
npm ERR! notarget Valid install targets:
npm ERR! notarget 2.0.0, 2.0.0-beta-7, 2.0.0-beta-6, 2.0.0-beta-5, 2.0.0-beta-4, 2.0.0-beta-3, 2.0.0-beta-2, 1.1.3, 1.1.1, 1.1.0, 1.0.3, 1.0.2, 1.0.1, 1.0.0, 0.1.1-0, 0.1.0, 0.0.4-4, 0.0.4-3, 0.0.4-2, 0.0.4-1, 0.0.4-0, 0.0.3, 0.0.2-1
npm ERR! notarget
npm ERR! notarget This is most likely not a problem with npm itself.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/andrew/Development/horizon/npm-debug.log

@marshall007
Copy link
Contributor

@abecks yea, I was wrong. I thought you could use that shorthand, but apparently that only works for actual npm releases, not git branches. @lirbank has a workaround in #790.

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

No branches or pull requests

3 participants