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 PrependPlugin #1839

Merged
merged 2 commits into from
Jan 24, 2022
Merged

add PrependPlugin #1839

merged 2 commits into from
Jan 24, 2022

Conversation

Code-Hex
Copy link
Contributor

@Code-Hex Code-Hex commented Jan 24, 2022

This PR should only be merged after #1838 is merged. #1838 reverts #1717

  • Some plugins that worked in versions prior to v0.15, which generated code based on a previously generated model, no longer work above v0.15.
    • gqlgen/api/generate.go

      Lines 21 to 32 in 8b25c9e

      plugins := []plugin.Plugin{}
      if cfg.Model.IsDefined() {
      plugins = append(plugins, modelgen.New())
      }
      plugins = append(plugins, resolvergen.New())
      if cfg.Federation.IsDefined() {
      plugins = append([]plugin.Plugin{federation.New()}, plugins...)
      }
      for _, o := range option {
      o(cfg, &plugins)
      }
  • I think we shouldn't implicit breaking changes should be made
  • If we want to achieve the behavior @Erwin-k claims, we should create a new API.
    • e.g. func PrependPlugin

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Jan 24, 2022

Coverage Status

Coverage increased (+0.03%) to 74.997% when pulling 4196844 on Code-Hex:patch-1 into fcee4c4 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Thanks for your work on this. Can you please add test coverage for your new PrependPlugin?

It seems like to make the modelgen documentation example work we do need AppendPlugin functionality as mentioned by @Erwin-k in #1717.
Which of those two behaviors "AddPlugin" should default to is more of an open question. Breaking changes are to be avoided, but I would like to know which known plugins require which behavior. Do you have examples of either?

@Code-Hex
Copy link
Contributor Author

Code-Hex commented Jan 24, 2022

@StevenACoffman I don't know why you are of the opinion that it is unresolved.

Obviously, the default should be the behavior before the breaking change occurs. It works on all versions except v0.15

Which of those two behaviors "AddPlugin" should default to is more of an open question. Breaking changes are to be avoided, but I would like to know which known plugins require which behavior.

I'm going to write a test for PrependPlugin, but I think that reverting the AddPlugin function should be done as soon as possible. (Regardless of the test)

@ghost
Copy link

ghost commented Jan 24, 2022

@Code-Hex I don't remember it exactly anymore but at the time I pushed this change it seemed that the plugins were being used in reverse order (by gqlgen). The former plugins would overwrite results of latter plugins (in the array), so prepending plugins would give desired behaviour for user using "addPlugin". Atleast that was what I thought was happening.
Maybe it wasn't the case.

@StevenACoffman perhaps it only applied to the modelgen hook or something? I noticed the documentation for modelgen is using "replacePlugin" now.

@Code-Hex
Copy link
Contributor Author

@Erwin-k Even if this were the case, it would not be a reason to implicitly change the behavior already provided in other versions.

The former plugins would overwrite results of latter plugins (in the array), so prepending plugins would give desired behaviour for user using "addPlugin". Atleast that was what I thought was happening.
Maybe it wasn't the case.

Look at the API provided by Go itself. Create new APIs without breaking the logic already provided. Always use the "Deprecated" comment even if you are going to deprecate.

@StevenACoffman StevenACoffman merged commit 7cefef2 into 99designs:master Jan 24, 2022
@StevenACoffman
Copy link
Collaborator

While avoiding breaking compatibility with existing usage is always the goal, mistakes happen. As mentioned in #1717 at the time I deemed it unlikely that anyone was relying on behavior that had changed from even earlier releases. These changes had then caused the documented example to fail. It also seemed that AddPlugin was somewhat counterintuitive in that it seemed that the plugins were being applied in reverse order, but this wasn't well documented or explicit.

Thanks to your work, people can now explicitly choose the behavior that they want. We can improve documentation on whether or not the plugins are applied in reverse/forward order (PRs welcome).

Please further consider contributing test cases for any uncovered areas of the code that you would like to avoid us unintentionally breaking compatibility with.

I am sorry for any inconvenience this may have caused you. Please also grant fellow open-source contributors the benefit of assuming only the best of intentions.

I will make a new v0.16.0 release with these changes shortly.

@Code-Hex Code-Hex deleted the patch-1 branch January 24, 2022 23:57
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.

3 participants