search

adaltas / node-http-status

Utility to interact with HTTP status code in Node.js
Other
441 stars 55 forks source link

Error TS2714 with TypeScript definition file #27

Closed TazmanianD closed 6 years ago

TazmanianD commented 6 years ago

I think there's an issue with the TS definition file that was added recently in 1.1.1. I did an npm update today and now I'm getting the following build error. This error does not occur in using version 1.1.0.

We're using TypeScript 2.8.3.

C:/Work/Journeys/radar/client/node_modules/http-status/lib/index.d.ts(1,10): error TS2714: The expression of an export assignment must be an identifier or qualified name in an ambient context.

wdavidw commented 6 years ago

I am no TypeScript expert, anyone could help here?

TazmanianD commented 6 years ago

I'm not an expert on .d.ts files but I think the basic problem here is that .d.ts files are only supposed to describe the shape of a module but not of any of the implementation details. Each .d.ts file is entirely for compile only purposes and no code should exist in them that you'd expect to be used in any transpiled output. Here, you've got strings like "Permanent Redirect" but those strings aren't going to end up anywhere. Those strings are implemented in the actual JavaScript for your library.

You don't actually need to include TS definitions as part of your module if you're not using TS and can't really maintain as it would needed. I'm always a fan of module authors including them because then the TS definitions stay in sync with the actual code but most libraries out there have TS definitions created separately as part of the DefinitelyTyped library. In fact, someone has already done that for http-status and you can see their version of this file here: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/http-status.

You might be able to just copy that file into your library although I'm not sure if it would be different since it would be part of the module it describes instead of an outside module.

One other thing that would be great if you decide to include the TS definitions in your library is other libraries will upload dummy definitions to DefinitelyTyped that produce an error if you try to use them. If you try to use them, you'll end up with some message like, "You don't need to use this as the types are defined in the library itself."

Your easiest path here is to just remove the .d.ts file and rely on the DefinitelyTyped one.

knor-el-snor commented 6 years ago

I fixed the issue in https://github.com/adaltas/node-http-status/pull/28. Tested it locally, and it works now. Although the remarks of TazmanianD are correct, using the DefinitelyTyped library to keep a cleaner overview. But with the PR I sent, the package will not break typescript compilation anymore

wdavidw commented 6 years ago

@knor-el-snor, I have been trying without success to add typescript to the mocha based test suite, with typescript being defined as a dev dependency. I will be happy to get help in case you have experience in this field. I will be beneficial to my others packages (csv, printf, ...) and I will be much more confident making TypeScript related changes.

knor-el-snor commented 6 years ago

@wdavidw I personally use jest for testing, but have used mocha with typescript in the past. But I actually do not have any coffeescript experience. I usually write my own modules in Typescript itself where you can put a transpiler before running the tests like babel-node or ts-jest

wdavidw commented 6 years ago

You dont need to use coffeescript, you could point to the generated js file directly and i'll deal with that. Otherwise, just requiring a coffescript should work transparently like if it was its js alternative. Mocha is already configured to transpile coffee file transparently.

I used to have some code where I declared the TypeScript in the package.json file and in mocha but it was no close from working and I cleaned up everything. This package is a good place to try to make it work because it is small.