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

Fix options getting swallowed by client preset #9496

Closed
wants to merge 1 commit into from

Conversation

jwueller
Copy link

Description

This deals with #8576, #8993 and #9104 by passing through all options by default instead of only allowing a small subset with no way to override that behavior. The original code doesn't give a rationale for doing this.

A few computed values are inserted into the config by the preset, and those will keep overriding the user options as of right now, because I assume that they required for the preset to work as expected.

This change should only be noticeable to users that were already passing options that didn't work/exist before. It basically has the same impact as adding a new option.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • yarn test doesn't show additional failures (there were some pre-existing ones before this change though)
  • Added a test for using dedupeOperationSuffix with the client preset (failed before, passes after)

Test Environment:

  • OS: Linux 6.3.5-arch1-1 Fill README #1 SMP PREEMPT_DYNAMIC Tue, 30 May 2023 13:44:01 +0000 x86_64 GNU/Linux
  • @graphql-codegen/client-preset@4.0.0
  • NodeJS: v20.2.0

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

@changeset-bot
Copy link

changeset-bot bot commented Jun 10, 2023

⚠️ No Changeset found

Latest commit: 9644912

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@jwueller jwueller marked this pull request as ready for review June 10, 2023 16:27
@jwueller
Copy link
Author

I'm not certain whether I should be the one adding a changeset to this. This would probably be a "patch" version bump based on my impact assessment.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 12, 2023

This is not a bug, but a strategic decision. We decided that we don’t allow all typescript plugin options, as we want to be more opinionated and later on replace the internals of the preset anyways to no longer use the typescript and typescript-operations plugins.

We are happy to gather feedback on all the configuration options people want and need (with specific use-cases mentioned) for consider enabling those specific configuration options (and also consider keeping them for the internals re-write).

@mogelbrod
Copy link

@n1ru4l Where do you want us to give feedback? Should we open a github issue for each individual setting we're missing and utilize the thumbs up feature? In that case issues like #8576 should probably be reopened.

@jwueller
Copy link
Author

@n1ru4l This is a very surprising decision, as it arbitrarily and severely reduces the usefulness of the preset.

I understand that you might want things to behave slightly differently in the future, but I don't see why it has to be forbidden until that decision actually gets made. It's going to be a backwards-incompatible change either way.

This is compounded by issues with the opinionated approach seemingly not getting addressed for a few months (see #9104, #8993 and #8576), which is presumably due to you having more important things to deal with. I feel like this change could reduce your workload in that regard, because it doesn't require dealing with a new issue and PR every time someone needs another option. This is assuming the user even bothers making an issue for it.

If I'm not able to override the preset in any way, I will unfortunately have to replace it entirely, even though it has been very useful up to this point.

@2snEM6
Copy link

2snEM6 commented Jun 12, 2023

One very useful option that doesn't work right now is the relay operation optimizer

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 12, 2023

@mogelbrod In the pinned issue #8562

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