Update exceptions with stacktraces

[ueman]

trafficstars

The qgl suite includes now stacktraces in LinkException (see this PR https://github.com/gql-dart/gql/pull/331). Since that's a breaking change, I've updated it here.

I'll update this PR once the changes from gql make it to a stable version, but until then, I guess, it doesn't make much sense to merge this.

Breaking changes

This might be considered a breaking change, but I guess most users aren't using LinkExceptions themselves.

Fixes / Enhancements

• Fixed usages of LinkException

[budde377]

Cheers! I don't think we can release this as a stable release before our dependencies stabilise.

[ueman]

Cheers! I don't think we can release this as a stable release before our dependencies stabilise.

Yes, and I don't expect you too. I just wanted to update graphql, too, since I landed that change in gql.

I'll come back to this when it's stable. If I somehow miss it, feel free to ping me.

[ueman]

In the meanwhile you can follow the guide https://github.com/zino-hofmann/graphql-flutter/blob/main/docs/dev/MAINTAINERS.md to fix your commit message, we do not have breaking change so we may work around a possible solution.

Of course, I changed it and force pushed to the branch.

In addition, I think we may think a solution to produce warning a runtime with dart compiler that alert the user that there is a new feature that it is coming.

I'm not sure if that's possible. I'm pretty sure user will have to accept, that they have to update the gql dependencies to use graphql with these proposed changes.

[vincenzopalazzo]

I'm not sure if that's possible. I'm pretty sure user will have to accept, that they have to update the gql dependencies to use graphql with these proposed changes.

All is possible with the code, we need just to experiment a little bit before putting out a library with breaking changes.

You can get the current stack trace and make the propriety optional https://api.dart.dev/stable/2.3.0/dart-core/StackTrace/current.html

If you have no time i can spend more time on it when I found some slot in my todo list

[ueman]

I'm not sure if that's possible. I'm pretty sure user will have to accept, that they have to update the gql dependencies to use graphql with these proposed changes.

All is possible with the code, we need just to experiment a little bit before putting out a library with breaking changes.

You can get the current stack trace and make the propriety optional https://api.dart.dev/stable/2.3.0/dart-core/StackTrace/current.html

If you have no time i can spend more time on it when I found some slot in my todo list

I'm pretty strongly against using StackTrace.current as default because it would just be misleading. By using current you would imply that the error was thrown at a different location than it was really thrown. That's why I choose null, but I guess StackTrace.empty would also be fine, because then we could make that property non-nullable. This is already not a breaking change anymore, we just need to figure which solution is the better one.

What's left is the breaking change in the gql libraries. Because graphql can either work with gql versions before their breaking change, or it can work with gql versions after their breaking change. But not both. However, I doubt that many people are using enough of gql to hit any of their breaking change, and thus it might practically not be a breaking change.

[budde377]

I don't think we should be too afraid of breaking changes and a new major version. These are great changes which a new major version can help raise awareness around.

We're not running out of version numbers anytime soon.

[vincenzopalazzo]

I don't think we should be too afraid of breaking changes and a new major version. These are great changes that a new major version can help raise awareness around. We're not running out of version numbers anytime soon.

I agree with this, and I'm not afraid to finish the number :) but I'm afraid to bump a new major release each time that a package under us put a new random breaking change only because the semver v2 tells us to bump a new version.

I would like that a major version contains major improvement inside the library and not just keep going by increasing the version number without any sense.

I'm pretty strongly against using StackTrace.current as default because it would just be misleading. By using current you would imply that the error was thrown at a different location than it was really thrown. That's why I choose null, but I guess StackTrace.empty would also be fine because then we could make that property non-nullable. This is already not a breaking change anymore, we just need to figure out which solution is the better one.

Mine was just an example to prove that avoiding introducing a not useful breaking change is possible in this case, for me any dummy value is fine, the user that cares about the stack trace will override the value, but the user that has no time to update to a new breaking change library (because people work like a boolean variable "if there is a breaking change I will not update because I have no time').

My position will not lock the final decision, but I strongly support the idea that we need to make fewer mistakes if it is possible, and moving the breaking change from a dependency to us for me is a bad decision because we can easily introduce a deprecation period with simply use a not required optional parameter.

If the user sees the wrong stack trace we help the user with more docs and a @deprecated field

[knaeckeKami]

Just FYI: I released gql_link 0.5.0 as "stable" (well, still major version zero, but not a prerelease...) according with updates to links that make use of exceptions.

[ueman]

Just FYI: I released gql_link 0.5.0 as "stable" (well, still major version zero, but not a prerelease...) according with updates to links that make use of exceptions.

Thanks for the update, I appreciate it. I think normalize still needs a new release which makes use of the new gql version. Otherwise, there will be dependency conflicts.

[knaeckeKami]

True, released normalize 0.7.1 which supports gql ^0.14.0

[vincenzopalazzo]

In the commit history is missed the deprecated entry to be able to log it in the changelog

[codecov[bot]]

Codecov Report

Merging #1197 (71cd486) into main (425f648) will decrease coverage by 0.12%. The diff coverage is 46.15%.

@@            Coverage Diff             @@
##             main    #1197      +/-   ##
==========================================
- Coverage   59.61%   59.49%   -0.13%     
==========================================
  Files          41       41              
  Lines        1684     1691       +7     
==========================================
+ Hits         1004     1006       +2     
- Misses        680      685       +5     

Impacted Files	Coverage Δ
packages/graphql/lib/src/core/query_manager.dart	76.35% <ø> (ø)
...es/graphql/lib/src/exceptions/exceptions_next.dart	16.07% <14.28%> (-0.91%)	:arrow_down:
packages/graphql/lib/src/exceptions/network.dart	18.18% <16.66%> (-4.05%)	:arrow_down:
...graphql/lib/src/cache/_normalizing_data_proxy.dart	92.98% <50.00%> (ø)
...ackages/graphql/lib/src/exceptions/network_io.dart	37.50% <66.66%> (ø)
...es/graphql/lib/src/core/_query_write_handling.dart	74.07% <80.00%> (+0.99%)	:arrow_up:
...ackages/graphql/lib/src/exceptions/exceptions.dart	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ueman]

In the commit history is missed the deprecated entry to be able to log it in the changelog

I'm not quite sure what you mean by that. What's now deprecated?

[vincenzopalazzo]

I'm not quite sure what you mean by that. What's now deprecated?

We discussed this in the thread, before making the breaking change in the next few releases, we should deprecate the filed to alert the user that we are going to add a breaking change and to do so in this case we need to add an deprecated(graphql): in order to generate the changelog.

Here we are going to deprecate the constructor if this is what you are asking!

You squash the change, and the review falls under your change, otherwise, you can make 2 commits where the 1th one is your change, and the 2th is the deprecation filed.

I leave this up to you on how to achieve this

[ueman]

@budde377 @vincenzopalazzo This is now ready

[ueman]

Looks Good to me thanks for your work and sorry for the delay!

Last call about this PR is from @budde377 :)

You're welcome. I'm happy to help out

[vincenzopalazzo]

I merge it now, and I will test it later if this will break somethings and try to introduce other fix to deprecate this feature, before introduce the big refactoring on the web socket I do not think that it is a good idea bump a new major release, but I will not lock no other choice, up to you.

I like only that the the graphql_flutter version 6 is a major improvement from the actual version tagged and right now from the version 5 couple of minor fix here and there other than feature like hooks. However, bumping a new major release now for just an exception that no one will use other than print on with some fancy logger I think it is no useful