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

Remove typescript and tslint from peerDependencies #109

Closed
wants to merge 2 commits into from

Conversation

marcofugaro
Copy link

@marcofugaro marcofugaro commented Mar 29, 2018

Was also discussed in #102,

Our problem with those peerDeps, is that we optionally use typescript in our build process, so just by having this package in the dependencies, it throws the error typescript is required.

screen shot 2018-03-29 at 11 26 03

It should be like ts-loader, typescript is required but it's up to the user to install it.

Thre README is fine since it says it explicitly to install also typescript.

Closes #102

@piotr-oles
Copy link
Collaborator

If we want to remove it from peerDeps, we should add some checks (for typescript package and it's version) in the plugin code to inform user that typescript is not available and he has to install it to use this plugin. :)

@SimonSchick
Copy link
Contributor

@nickserv
Copy link
Contributor

nickserv commented Aug 8, 2018

Unfortunately optionalDependencies don't work with peerDependencies, they have to be direct dependencies of this package and not its dependent, which isn't helpful.

@sebinsua
Copy link

sebinsua commented Oct 18, 2018

This isn't correct.

peerDependencies will not cause an error about a module not existing. All it does is cause a warning to be printed during installation telling you what version range is expected. It's purely informational to help developers understand what version of the TypeScript compiler is supported. It's your choice whether you then install this.

If you want to not get runtime errors, you will need to write something like this.

let typescript;
try {
  typescript = require('typescript');
} catch (err) {
  console.warn('TypeScript needs to be installed for fork-ts-checker-webpack-plugin to do anything.');
}

// ...somewhere deep within the plugin...

if (typescript) {
  // Conditionally decide what to do if it is installed here.
}

@glen-84
Copy link

glen-84 commented Dec 12, 2018

All it does is cause a warning to be printed during installation telling you what version range is expected.

... and this when using npm list --depth=0:

npm ERR! peer dep missing: tslint@^4.0.0 || ^5.0.0, required by fork-ts-checker-webpack-plugin@0.5.1

I'm not using the tslint integration, and it even requires setting tslint: true, so I'm not sure why it's listed as a peer dependency.

@sebinsua
Copy link

sebinsua commented Dec 12, 2018

If you remove the peerDependency you no longer have a way to inform the user of what version range of a dependency is supported.

I think what you're implicitly asking for is optional peerDependencies. This feature is in discussion.

@marcofugaro
Copy link
Author

Closing because it was done in #201

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 this pull request may close these issues.

6 participants