cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
73 stars 20 forks source link

typescript problem #26

Open rafaelyates opened 7 years ago

rafaelyates commented 7 years ago

looks like we currently does not have the types for cfenv

$ npm install @types/cfenv --save
npm ERR! code E404
npm ERR! 404 Not Found: @types/cfenv@latest

Thanks in advance

drnic commented 6 years ago

Alternately I think we can put a typescript definition index.d.ts file in this repo.

pmuellr commented 6 years ago

That works for me.

To make this work right, since I don't use TypeScript, we'll have to add some kind of test that ensures the contents of the typescript def file are correct. Not sure quite what that would be - if there's a "lint" option on the typescript compiler, or if we should write a test in typescript that would pull in the def file and then ensure it's correct.

I really don't want to be in a position of adding a file that folks - especially me! - don't end up checking before committing new code. :-)

I think I was poking around this area in an unrelated GH repo, didn't get any responses on the best way to handle this sort of thing, but I'm sure there's a low-cost solution to be found. Pre-req'ing the TS compiler as a devDep isn't a problem.

pmuellr commented 6 years ago

Also, non-intrusive flow bits are welcome as well, same constraints. Must provide some way of checking the flow bits via tests of some sort. devDeps for flow bits aren't a problem.

drnic commented 6 years ago

I've started a branch to rewrite the library in typescript [1] - which has a cfenv.d.ts file as a byproduct [2].

[1] https://github.com/drnic/node-cfenv/blob/typescript/lib-src/cfenv.ts [2] https://github.com/drnic/node-cfenv/blob/typescript/lib/cfenv.d.ts

I'll keep working on it. At this moment, deploying the lib/server.js sample app to CF is working.

I've not yet rewritten server.coffee nor the test suite.

@pmuellr are you interested in moving the library towards typescript rather than coffeescript? Or just want the cfenv.d.ts file for typecript users?

drnic commented 6 years ago

cfenv-typescript

Sweet sweet IDE assistance.

Use my branch with:

  "dependencies": {
    "cfenv": "drnic/node-cfenv#typescript",
drnic commented 6 years ago

@pmuellr I tried to run the tests on my branch but I think a) I'm not familiar with mocha nor your https://github.com/pmuellr/jbuild; b) my code paths for local files/islocal isn't working wrt to the tests.

Perhaps we can pair on some fixes and chat about the typescript implementation?

I'd like the current .coffee tests to pass before rewriting them (and accidentally writing tests that pass; but not the right tests).

pmuellr commented 6 years ago

I'm not much of a CF user anymore, don't have much skin in the game, happy to pass the torch to folks wanting better. TS seems perfectly reasonable to me. LMK and I can folks as maintainers of the npm package.

We can certainly get rid of jbuild and use an alternate method of "building", assuming the source will be in TS and then compiled to JS.

I'll check the fork and see if anything obvious pops out to me re: the tests. Otherwise, can certainly pair, maybe twitter dm me @pmuellr to find a time?

pmuellr commented 6 years ago

I have some comments to make on the ongoing branch, maybe create a PR you can keep pushing to, to make discussion easier?

There will need to be some kind of "build" command, I guess preferrably as an npm script like the current build script below, to invoke ./node_modules/.bin/tsc. Eg, the property value for build can prolly just be tsc. I figured just running tsc would end up recompiling everything, and did, but need something simpler than the mouthful above.

https://github.com/cloudfoundry-community/node-cfenv/blob/3490eda2643cb044c5a19d47eaa42cb2fbe194f8/package.json#L13-L18)

I then ran the tests via npm run test, looked at a couple of the errors, seemed to be understandable in terms of figuring out how to fix them. Eg, the default "app" name seems to be "ignoreme" in this branch, which is not expected, and hence the tests checking that are failing. etc