Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

is @types/geojson better off as a devDependency? #305

Closed
jgravois opened this issue Apr 26, 2017 · 5 comments
Closed

is @types/geojson better off as a devDependency? #305

jgravois opened this issue Apr 26, 2017 · 5 comments

Comments

@jgravois
Copy link
Collaborator

a few users reported errors when attempting to install v1.0.8 (see #304) because they are using an internal npm registry.

this got me thinking that @types/geojson should really only be a devDependency because Terraformer doesn't actually require that it be present to function.

related discussion: microsoft/types-publisher#81 (comment)

@darcyparker is there a way that TypeScript developers can get what they need for Terraformer automatically without those additional libraries being downloaded by everyone?
@darcyparker
Copy link
Contributor

@jgravois - Personally I am okay with adding @types/geojson as a devdependency in Terraformer and my repos that use it. But there should be instructions for typescript developers that want to use Terraformer because if they do npm install --save Terraformer, they won't get the required @types/geojson and they will get errors about the typescript definitions in Terraformer.

I suppose its a trade off/compromise for someone. People using an internal npm registry could host the new required dependency or people using the typescript definition could work around issue by knowing to add the devdependency manually in each repo where they use Terraformer. I think the right choice is to have the library declare its dependency on the typescript definition. But with appropriate instructions I could take the compromise if its more pain to update an internal npm repository.

@darcyparker
Copy link
Contributor

Just wanted to highlight this comment from related discussion. microsoft/types-publisher#81 (comment) I think it summarizes things well. The best choice for a lib is --save but it can be worked around if typescript users know they have to manually install both Terraformer and @types.geojson

@jgravois
Copy link
Collaborator Author

I suppose its a trade off/compromise for someone.

isn't everything. 😄

my guess is that the number of TypeScript Terraformer developers outnumber the vanilla JS developers using private npm registries. for now i'll just leave this issue open to collect 👍 and 👎 s.

@darcyparker
Copy link
Contributor

👍 === For leaving things as is. @types/geojson is a dependency for terraformer.d.ts and should be saved as such in package.json
👎 === For moving @types/geojson to a devDependency and add instructions for typescript users about the need to manually npm install --save terraformer @types/geojson to use Terraformer because they will require @types/geojson. This isn't ideal because people have to be aware of the dependency. But it may help users of private npm registries not have to update the registry with this new dependency.

My vote is 👍 . But I can live with the alternative.

@harmon
Copy link

harmon commented May 11, 2017

Thanks for considering this issue!

This broke our installation of Sequelize. We are using a private server, and the error is SUPER not helpful. Please err on the side of VanillaJS, as a few big projects seem to be using this library as a dependency, and most devs aren't using Typescript (although it would be nice!)

I installed Sequelize in a new project using our private server just 3 weeks ago, and it worked just fine. But today, not so much:

$ npm install sequelize
npm ERR! registry error parsing json
npm ERR! Darwin 16.5.0
npm ERR! argv "/usr/local/Cellar/node@6/6.10.0/bin/node" "/usr/local/bin/npm" "install" "sequelize"
npm ERR! node v6.10.0
npm ERR! npm  v3.10.10

npm ERR! Unexpected token < in JSON at position 0
npm ERR! <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
npm ERR! <html><head>
npm ERR! <title>404 Not Found</title>
npm ERR! </head><body>
npm ERR! <h1>Not Found</h1>
npm ERR! <p>The requested URL /private/api/npm/npm-virtual/@types/geojson was not found on this server.</p>
npm ERR! </body></html>
npm ERR!
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     npm-debug.log

Regards,
Harmon

@jgravois jgravois mentioned this issue Dec 28, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@harmon @darcyparker @jgravois