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

Add ts-node as a peerDependency #8779

Merged
merged 1 commit into from Jan 4, 2023
Merged

Conversation

louisscruz
Copy link
Contributor

@louisscruz louisscruz commented Dec 30, 2022

Description

Related #8685

This PR fixes the above issue by making ts-node a peerDependency of the CLI package. ts-node is necessary as a peerDependency because of the use of cosmiconfig-typescript-loader and its own peerDependency on ts-node. The peerDependency specification added in this PR matches that of cosmiconfig-typescript-loader. This PR also updates various places in documentation where ts-node should be installed by the consumer.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I've used the packageExtensions workaround in the issue with this configuration and it works.

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • [-] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [-] I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

I skimmed the contributing guidelines given my current time constraints. Given this is a pretty tiny fix and that there's already an open issue (or two?) for this problem, I assume it might be considered. Something like this PR is the fix. If this PR isn't accepted, feel free to copy whatever from this PR.

@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2022

🦋 Changeset detected

Latest commit: 7d27f55

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@graphql-codegen/cli Patch
@graphql-cli/codegen Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sjdemartini
Copy link

Per this #8685 (comment), shouldn't the peerDependencies be updated here to include @types/node and typescript as well (not just ts-node)?

@louisscruz
Copy link
Contributor Author

@sjdemartini Yeah, those need to be dealt with in some way too, but @graphql-codegen/cli still works with those warnings. The current ts-node issue prevents @graphql-codegen/cli from working at all in any project using PnP-style package managers. So I'm hoping to leave those issues out of scope of this PR.

There are also some peer dependency issues with some of the dependencies pulled in by @graphql-codegen:

➤ YN0002: │ @graphql-tools/graphql-tag-pluck@npm:7.4.2 [d8dd2] doesn't provide @babel/core (p28834), requested by @babel/plugin-syntax-import-assertions
➤ YN0002: │ graphql-config@npm:4.3.6 [e9352] doesn't provide @types/node (pedcf8), requested by cosmiconfig-typescript-loader
➤ YN0002: │ graphql-config@npm:4.3.6 [e9352] doesn't provide @types/node (pb5e49), requested by ts-node
➤ YN0002: │ graphql-config@npm:4.3.6 [e9352] doesn't provide typescript (pd6b97), requested by cosmiconfig-typescript-loader
➤ YN0002: │ graphql-config@npm:4.3.6 [e9352] doesn't provide typescript (pf6c69), requested by ts-node

There might need to be some larger discussion around what strategy should be taken for this problem. There should probably be something in place to prevent these sorts of issues from creeping into the codebase in a more general way. It might be nice taking care of those when adding a yarn doctor check to CI that fails on any peer dependency issues. If this repo were upgraded to modern Yarn, that might help catch these module resolution kinds of problems earlier.

Copy link
Collaborator

@saihaj saihaj left a comment

Choose a reason for hiding this comment

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

hmm yeah we need to find a way to support yarn pnp 🤔

@saihaj saihaj merged commit ad5d833 into dotansimha:master Jan 4, 2023
@saihaj
Copy link
Collaborator

saihaj commented Jan 4, 2023

@sjdemartini
Copy link

Re underlying dependencies having dependency issues, the underlying graphql-config package has since added its missing @types/node and typescript peer deps in kamilkisiela/graphql-config#1178, so graphql-code-generator should presumably update its graphql-config package dep to 4.4.0+, and specify its own peer deps for @types/node and typescript, and it will seemingly resolve most of the remaining dependency issues.

@yura3d
Copy link

yura3d commented Jan 5, 2023

@saihaj I got strange result after upgrading to @graphql-codegen/cli@2.16.3

On previous versions, there was only 1 warning about ts-node:

warning "@graphql-codegen/cli > cosmiconfig-typescript-loader@4.3.0" has unmet peer dependency "ts-node@>=10".

Now I have 2 same warnings:

warning " > @graphql-codegen/cli@2.16.3" has unmet peer dependency "ts-node@>=10".
warning "@graphql-codegen/cli > cosmiconfig-typescript-loader@4.3.0" has unmet peer dependency "ts-node@>=10".

Since @graphql-codegen/cli doesn't use ts-node, but relies on cosmiconfig-typescript-loader as dep, which in turn relies on ts-node as peer dep, there are only 2 paths to resolve these warnings:

  1. move ts-node from peer deps to deps;
  2. remove ts-node from peer deps (we should not specify third side dependencies, otherwise we will must control their versions' constraints stay sync) and specify in the docs that ts-node should be installed together with codegen packages (done by this PR).

I think the 1st path is preferrable.

@saihaj
Copy link
Collaborator

saihaj commented Jan 6, 2023

@yura3d the reason we do 2 is to support yarn PNP issues cause of which this change was introduced. I don't like 1

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.

None yet

4 participants