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 should be a peerdependency #839

Closed
arcanis opened this issue Sep 22, 2018 · 5 comments · Fixed by #841
Closed

TypeScript should be a peerdependency #839

arcanis opened this issue Sep 22, 2018 · 5 comments · Fixed by #841

Comments

@arcanis
Copy link
Contributor

arcanis commented Sep 22, 2018

Expected Behaviour

ts-loader shouldn't rely on undefined behaviors

Actual Behaviour

ts-loader currently relies on the hoisting in order to have access to the typescript package

The fix is pretty simple: typescript just has to be listed as a peerDependencies of ts-loader (rather than just a devDependencies). This will guarantee that it will then get the exact same instance than its parent. It can be listed as both if you want to also install it by default on dev environments.

Steps to Reproduce the Problem

It causes issues with package trees that do not support hoisting, such as Yarn Plug'n'Play.

Location of a Minimal Repository that Demonstrates the Issue.

Because Plug'n'Play has a fallback mechanism, hitting this case is a bit more difficult to reproduce - but basically, if you have a workspace that depends on both ts-loader and typescript, it won't work (because ts-loader won't be allowed to access typescript as it's not one of its dependencies, and because the fallback won't kick in since typescript will be in a workspace rather than the top-level module).

@johnnyreilly
Copy link
Member

I'm open to this but I'm not sure how feasible it is. We build ts-loader with a specific version of TypeScript (latest and greatest usually) so we have a high version devDependency. But typically TypeScript can be used with a whole number of different versions of TypeScript. I'm not sure we can have it listed as a peer dependency as well as a devDependency with different versions.

BTW yarn is fine; I can say that as that's what I use!

@arcanis
Copy link
Contributor Author

arcanis commented Sep 23, 2018

I'm not sure we can have it listed as a peer dependency as well as a devDependency with different versions.

It shouldn't be an issue semantically speaking - devDependencies are only installed for the top-level package, which by definition cannot see their peer dependencies satisfied. Something like this should give you the behavior you wish:

{
  "devDependencies": {
    "typescript": "latest"
  },
  "peerDependencies": {
    "typescript": "*"
  }
}

BTW yarn is fine; I can say that as that's what I use!

Glad to hear that! I'm actually one of Yarn's maintainers 😄

We've recently unveiled the Plug'n'Play project, which will (optionally for now) enforce strict boundaries between packages (à la pnpm) and will allow the installs to become much faster and safer. It might cause some issues when packages don't list their whole set of dependency, hence my report 🙂

@johnnyreilly
Copy link
Member

Would you like to submit a PR that adds peerDependencies / devDependencies as suggested above?

I'll take it for a whirl. If it works without problems then I'd be happy to merge it 😄

@marvinhagemeister
Copy link

We're using the same strategy for some private packages at work. It's perfectly fine to specify something in both devDependencies and peerDependencies 👍

@arcanis
Copy link
Contributor Author

arcanis commented Sep 25, 2018

#841 adds the dependency to the peerDependencies field! 🙂

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

Successfully merging a pull request may close this issue.

3 participants