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

tsify should use typescript as a peerDependency #85

Closed
basarat opened this issue Oct 5, 2015 · 9 comments
Closed

tsify should use typescript as a peerDependency #85

basarat opened this issue Oct 5, 2015 · 9 comments

Comments

@basarat
Copy link
Member

basarat commented Oct 5, 2015

Otherwise the user installed version isn't used
Code :

"typescript": "~1.6.2"

/cc @johnnyreilly

basarat added a commit to basarat/globalize-so-what-cha-want-typescript that referenced this issue Oct 5, 2015
* Fix TypeStrong/tsify#84
* Require typescript workaround for TypeStrong/tsify#85
* Ask browserify to transform the output file
@johnnyreilly
Copy link
Member

Given the changes to peerDependencies in npm 3 should this be a devDependency instead?

@nycdotnet
Copy link

Whatever the solution is for this should probably be the solution for grunt-ts too bearing npm 3 in mind.

I'm still annoyed they're doing this.

@blakeembrey
Copy link
Member

should this be a devDependency instead?

I don't think using devDependency would work for what you want, except in development here. Personally, I prefer the peerDependency changes - it was too easy to get into a broken state with npm before. Not saying this is the best solution, but personally, it is better. I've just opted to tell people to install what they want themselves and you could put TypeScript as a peer dependency for that (but I'd probably use * to avoid anyone running into issues installing).

@smrq
Copy link
Member

smrq commented Oct 7, 2015

I'm not really convinced that this is the correct course of action. There's lots of arguments for both sides happening across a lot of different threads in various repos, but ultimately the way I'm reading the comments from the npm team, dependency is the correct type for TypeScript.

npm/npm#6565 (comment)

In this case, "I need this thing directly available to me" applies to the TS compiler, whereas "if you want to use me, you need this thing available to you" applies to Browserify.

I'm open to further discussion, though.

Note that you can pass in whatever compiler you like with the typescript API option (the name of which I'm definitely open to changing per TypeStrong/discussions#1).

@smrq smrq added bug discussion and removed bug labels Oct 7, 2015
@blakeembrey
Copy link
Member

@smrq The issue is with dependency you can't actually pass a custom compiler using --typescript as you are using require - which would resolve to your own instance before the users instance. The only way to solve that and keep it as a dependency would be to resolve the users typescript instance starting two directories up.

Edit: The custom compiler being typescript which is the dependency here, that's kind of important to the first sentence.

@smrq
Copy link
Member

smrq commented Oct 7, 2015

@blakeembrey Well, I guess that's true if you're using the CLI. With the API it would work as I stated:

tsify/index.js

Lines 7 to 8 in f8105a6

var ts = opts.typescript || require('typescript');
if (_.isString(ts)) { ts = require(ts); }

As long as the caller passes require('typescript') and not just 'typescript', the module will be resolved relative to the caller and not re-resolved by tsify, which is what you'd expect.

@basarat
Copy link
Member Author

basarat commented Oct 7, 2015

You can leave it as it is, but I've added docs so that its clear that if they want to use their version they need to specify it : #87 🌹 Close this if you are okay with that 👍

@smrq
Copy link
Member

smrq commented Oct 8, 2015

Thanks for the docs update. I'll keep things as is for now but I'm keeping an eye on TypeStrong/discussions#1 and other discussion. I'm hoping that as npm 3 adoption settles down, a single dominant pattern gets established across the community.

@smrq smrq closed this as completed Oct 8, 2015
@basarat
Copy link
Member Author

basarat commented Oct 9, 2015

I'm hoping that as npm 3 adoption settles down, a single dominant pattern gets established across the community.

Just info that might already be common knowledge: There is no pattern for dev tools ... but for front-end libs peerDependency is still the way to go. One does not want people installing two version of react or jquery or insert famous lib here 🌹

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

No branches or pull requests

5 participants