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

typescript problem #26

Open
rafaelyates opened this issue Aug 25, 2017 · 8 comments
Open

typescript problem #26

rafaelyates opened this issue Aug 25, 2017 · 8 comments

Comments

@rafaelyates
Copy link

rafaelyates commented Aug 25, 2017

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
Copy link

drnic commented Jun 12, 2018

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

@pmuellr
Copy link
Member

pmuellr commented Jun 13, 2018

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
Copy link
Member

pmuellr commented Jun 13, 2018

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
Copy link

drnic commented Jun 13, 2018

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
Copy link

drnic commented Jun 13, 2018

cfenv-typescript

Sweet sweet IDE assistance.

Use my branch with:

  "dependencies": {
    "cfenv": "drnic/node-cfenv#typescript",

@drnic
Copy link

drnic commented Jun 13, 2018

@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
Copy link
Member

pmuellr commented Jun 14, 2018

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
Copy link
Member

pmuellr commented Jun 14, 2018

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.

node-cfenv/package.json

Lines 13 to 18 in 3490eda

"scripts": {
"build": "jbuild build",
"serve": "jbuild serve",
"test": "jbuild test",
"watch": "jbuild watch"
},
)

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

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