-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
|
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. |
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). |
@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. |
One very useful option that doesn't work right now is the relay operation optimizer |
@mogelbrod In the pinned issue #8562 |
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
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)dedupeOperationSuffix
with the client preset (failed before, passes after)Test Environment:
@graphql-codegen/client-preset@4.0.0
Checklist: