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

Typings for horizon #684

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Typings for horizon #684

wants to merge 12 commits into from

Conversation

beenotung
Copy link

Typings URL: https://github.com/beenotung/typed-horizon

Questions (for new typings):

  • Does the README explain the purpose of the typings and have a link
    to the JavaScript project?
  • Do the typings follow the source structure (e.g. index.js <->
    index.d.ts)?
    • no, the typing is in a single file
  • Are they external or global modules according the source (e.g. see
    README sources)?
    • they are global modules

**Typings URL:** https://github.com/beenotung/typed-horizon

**Questions (for new typings):**

* [x] Does the README explain the purpose of the typings and have a link
to the JavaScript project?
* [ ] Do the typings follow the source structure (e.g. `index.js` <->
`index.d.ts`)?
no, the typing is in a single file
* [x] Are they external or global modules according the source (e.g. see
README sources)?
they are global modules
@beenotung
Copy link
Author

The label should be typings request

how to set the label?

@blakeembrey
Copy link
Member

Definition files shouldn't contain values (https://github.com/beenotung/typed-horizon/blob/master/src/main.d.ts#L77-L80). Aside from that, is this for the browser API? Is everything global under horizon?

@beenotung
Copy link
Author

beenotung commented Aug 13, 2016

@blakeembrey: yes, it is for browser, and the npm test script suggests it to be global

changed to global typing;

refer to raw file directly:
eliminate the need to maintain a 'release branch' (reduce duplicated
storage);
@unional
Copy link
Member

unional commented Aug 13, 2016

The test script say it is global because you choose it to be global when you run the generator. (You probably selected "Script Tag" when it asked "How can the package be used?")

I checked the source and I think it is likely umd:
https://horizon.io/api/horizon/

// connect to the Horizon server, after Horizon has been loaded via
// <script> tag or require
const hz = new Horizon();

@unional
Copy link
Member

unional commented Aug 13, 2016

I need to find time to re-do part of the generator, or may be re-write it to be not depends on yeoman. It really tie my hand in properly refactoring the code. 😢

@beenotung
Copy link
Author

in this case, it should be in source (root folder) of npm?

@blakeembrey
Copy link
Member

Thanks. You can also use classes into of this style: https://github.com/beenotung/typed-horizon/blob/master/global/main.d.ts#L2-L28.

@unional
Copy link
Member

unional commented Aug 13, 2016

It depends on whom you want to support. If you want to support script tag usage, then this is fine.

If you also want to support npm (require) usage, then it is better to also have one for /npm/ folder.

@felixfbecker
Copy link
Contributor

This general question interests me aswell, where should typings for modules go that support both global and commonjs usage?

@beenotung
Copy link
Author

the current typing is for browser

I am not sure if the nodejs part is the same, might check out that part later on

I think horizon is not isomorphic, it seperate code under @horizon/server and @horizon/client

@unional
Copy link
Member

unional commented Aug 13, 2016

When talking about npm and require, it is not necessary just server side usage. You can perfectly use npm to get packages and develop your code, then use tools like browserfiy, jspm, webpack etc, and use it on browser.

@unional
Copy link
Member

unional commented Aug 13, 2016

@felixfbecker to both global and npm. The question is what are we going to do with umd typings in [email protected]

@beenotung
Copy link
Author

beenotung commented Aug 13, 2016

@unional I guess I know your meaning

but the client side horizon javascript is generated from live server on every request (i guess it embed some auth stuff)

and i tried to eval the "client side code" in nodejs and it doesn't really work (cannot setup connect to horizon) becuase it used window.localStorage internally

@unional
Copy link
Member

unional commented Aug 13, 2016

i tried to eval the "client side code" in nodejs

Nope that won't work. Your code will still need to be end up running on the browser, by either loading by script tag, requirejs, systemjs/jspm, etc.

@unional
Copy link
Member

unional commented Aug 13, 2016

In either way, if this is ready, we can merge this and you can create another PR if you want to support npm module usage.

@beenotung
Copy link
Author

@blakeembrey when using class, how to indicate the 'default function' ?
https://github.com/beenotung/typed-horizon/blob/master/global/main.d.ts#L7

I know using apply or call can do the trick, but it's not convenience for the user 😕

class Foo {
  apply(name:string):Bar;
}

@blakeembrey
Copy link
Member

Oh, I missed that line doing that trick. It's a really weird pattern to do that, so your current code looks right. Also, what about lazyWrites (https://horizon.io/api/horizon/#constructor) and doesn't that function take an argument like new Horizon()('messages')? Let me know if I'm reading the right thing 😄

@beenotung
Copy link
Author

the param in constructor is optional already 😄

new(param?:{...})

you reminded me the typing is not completed, i'm just working on it progressively as far as my project cover
i will update them later on

@blakeembrey
Copy link
Member

@beenotung I was trying to point to the method you wanted (https://horizon.io/api/horizon/#connect - https://github.com/beenotung/typed-horizon/blob/master/global/main.d.ts#L7) - shouldn't it take a table: string argument?

in order to import npm:rxjs
(instead of typings:rx-lite previously)
@beenotung
Copy link
Author

beenotung commented Aug 13, 2016

@blakeembrey are you talking about the call method?

export interface Horizon {
    call<A>(_this: Horizon, table: string): TableObject<A>;
    <A> (): TableObject<A>;
  }

it works, but the syntax when invoking the method is trivial 😢

let hz = new Horizon();
hz.call(hz,'tablename');

remark:
hz(a) === hz.call(hz,a)

@beenotung
Copy link
Author

@blakeembrey I just got you meaning, fixed :)
beenotung/typed-horizon@a3f86d2

@beenotung
Copy link
Author

when i use it in another project
this error shows up when compiling

typings/globals/horizon/index.d.ts(4,23): error TS1147: Import declarations in a namespace cannot reference a module.
typings/globals/horizon/index.d.ts(4,23): error TS2307: Cannot find module 'rxjs'.

how should I set the typings.json so that it will lookup under node_modules/ ?

@blakeembrey
Copy link
Member

  1. Don't do imports in a namespace
  2. This one is tricky because no solution here is ideal, but you can do npm:rxjs - it just needs to be installed by npm before resolving

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants