Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

chore(deps): Move typescript to peerDependencies #295

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented May 29, 2020

@ExE-Boss ExE-Boss requested a review from sandersn as a code owner May 29, 2020 15:43
@sandersn
Copy link
Member

I think it's actually the fault of dtslint's dependencies dts-critic and @definitelytyped/definitions-parser. dtslint needs to depend directly on typescript so that it can be run via package.json's bin entry.

@ExE-Boss
Copy link
Contributor Author

That will also work with peer dependencies.

@sandersn
Copy link
Member

#296 updated dtslint's dependencies. I published it and will watch DT to see if it works.

@sandersn
Copy link
Member

(It works locally.)

@Maxim-Mazurok
Copy link

Maxim-Mazurok commented May 30, 2020

I think it's actually the fault of dtslint's dependencies dts-critic and @definitelytyped/definitions-parser. dtslint needs to depend directly on typescript so that it can be run via package.json's bin entry.

That doesn't seem to be the case. Even with latest dtslint, I still can reproduce the issue.
Here's what happens:

  • I have [email protected] as a dependency of my project.
  • I'm running dtslint
  • dtslint imports and calls tslint
  • tslint doesn't have typescript dependency, so it uses my projects version to parse ts files and convert them into linkable structure
  • dtslint rule voidReturnRule also imports typescript but since dtslint has typescript dependency, it uses local ts version which has different enum values

Here's an illustration:
dtslint-2

@Maxim-Mazurok
Copy link

I just tried this fork (PR) on my project using npm link and it worked perfectly!
I was running it using bin like so:

./node_modules/.bin/dtslint types/gapi.client.abusiveexperiencereport

Copy link

@Maxim-Mazurok Maxim-Mazurok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the fix we need to make dtslint reliably working in graphql-js 👍

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this and ship it as soon as DefinitelyTyped/DefinitelyTyped#45226 is in.

@sandersn sandersn merged commit c63169c into microsoft:master Jun 2, 2020
@sandersn
Copy link
Member

sandersn commented Jun 2, 2020

OK, 3.6.10 is published, let me know if there are problems.

@IvanGoncharov
Copy link
Contributor

@sandersn It fixed every for graphql-js.
The next challenge is to understand why our typing is failing strict-export-declare-modifiers but I think it is a different bug.

@ExE-Boss ExE-Boss deleted the fix/issue-281 branch June 2, 2020 22:06
ghostd added a commit to ghostd/dom-testing-library that referenced this pull request Jun 10, 2020
It seems the prettier settings can conflit with the 'whitespace' rule of dtslint.

Also forces the dtslint version to be sure Travis use a version with this fix: microsoft/dtslint#295
mastermatt added a commit to mastermatt/nock that referenced this pull request Feb 24, 2021
`dtslint` moved typescript to a peer dependency so now we need to include
it in our package.json.
microsoft/dtslint#295
mastermatt added a commit to mastermatt/nock that referenced this pull request Feb 25, 2021
`dtslint` moved typescript to a peer dependency so now we need to include
it in our package.json.
microsoft/dtslint#295
mastermatt added a commit to nock/nock that referenced this pull request Feb 25, 2021
`dtslint` moved typescript to a peer dependency so now we need to include
it in our package.json.
microsoft/dtslint#295
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS versions for parsing source and linting rules differ
4 participants