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 peer dependency #697

Closed
arcanis opened this issue Oct 7, 2018 · 9 comments · Fixed by #758
Closed

TypeScript should be a peer dependency #697

arcanis opened this issue Oct 7, 2018 · 9 comments · Fixed by #758

Comments

@arcanis
Copy link

arcanis commented Oct 7, 2018

Previous discussion here: #315

The typescript package isn't listed as a peer dependency of ts-node. It kinda works because the hoisting breaks boundaries between packages, allowing such require calls, but it's unsafe and relies on an undefined behavior. Most package managers start to make a move against such patterns, so it'd be best to fix it 🙂

@blakeembrey mentionned that a possible problem with peer dependencies was that it would require "updating the peer dependency every single nightly release to account for prereleases in the range". It's only partially true:

  • The range specified in peerDependencies in only an hint used by package managers to warn the user when they install an incompatible version. It's not a hard requirement though, and no package manager will see it as an error to use an "incompatible" version.

  • The problem if there's one is a global problem and should be solved at a global scale (either in the semver standard or in the way the package managers interpret the peerDependencies constraints with regard to prereleases). Such workarounds, while useful to solve a given problem in a short timeframe, solve it at the wrong layer and may cause problems (as is the case here).

Note that we already slightly changed the semantic of the engines check in Yarn to also account for prereleases (starting from 0.26 iirc). I think we could do that with peer dependencies as well 🤔

@blakeembrey
Copy link
Member

Please note that it’s also possible to use this package without the TypeScript compiler. It sounds like this feature will be broken in the future. I’m definitely happy to change this and kill that feature for the next major release.

@arcanis
Copy link
Author

arcanis commented Oct 7, 2018

Do you have an example of the behavior expected in such a case?

I think peer dependencies shouldn't affect this much - at worst a warning will be displayed when TypeScript is missing, but functionally speaking it should be the same. We're actually discussing a similar use case (optional peer dependencies) at yarnpkg/yarn#6487

@blakeembrey
Copy link
Member

It’s the --compiler option. It defaults to typescript, but can be anything. In general this feature is probably unused by the majority (especially since TypeScript has native nightly support), which is why I’m happy to remove it. It seems like this package being able to resolve any other will be going away, so best to remove this before there’s too much confusion.

@MichalLytek
Copy link

So basically it's now impossible to have custom transformers using custom compilers like ttypescript?
https://github.com/cevek/ttypescript#ts-node

@blakeembrey
Copy link
Member

That does seem like a problem. What can be done to support that use case? I can certainly add the flag back for you before next release. Does your tooling wrap the existing compiler or use transforms? Maybe there’s a different feature we can expose for you?

@MichalLytek
Copy link

MichalLytek commented Oct 12, 2018

Basically, what ttypescript does is wrapping the tsc compiler to apply custom plugins/transformers that are supported by TypeScript only programmatically, like ts-transform-graphql-tag or ts-nameof.
See microsoft/TypeScript#14419 for more info why.

Webpack loaders and others have support for it, the problem is with node. I guess it's easier to restore the flag rather than reimplementing support for custom transformers in ts-node.

@blakeembrey
Copy link
Member

@19majkel94 Both are easy enough, I'd just look at keeping it as also a peer dependency. Hopefully that'll be ok for everyone all round. I'll reopen this to revert and make that change.

@blakeembrey blakeembrey reopened this Oct 13, 2018
@arcanis
Copy link
Author

arcanis commented Oct 13, 2018

Note that it doesn't have to be a peer dependency provided the resolution is done by the owner of the compiler.

If ts-node was to call require.resolve with a paths option that would only contain the cwd, it should then be able to pass the result to require without issues (the resolution would then be done against the dependencies of the package owning the cwd, not the dependencies of ts-node itself).

@blakeembrey
Copy link
Member

That’s great to know, thank you. It sounds like the optimal balance here.

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

Successfully merging a pull request may close this issue.

3 participants