improvement: add async?/1 to source protocol

[zachdaniel]

In order to make dataloaders easier to use in test, I've added an async?/1 function to the souurce protocol. In async_safely/3 (which is now async_safely/4) we default to async?: true meaning this should be backwards compatible with any direct usage of that function.

I've also added what would seem to me to be the proper default behavior for ecto and KV, which is that ecto will run synchronously if the repo is in a transaction, and KV is configurable at startup.

Feel free to tear it apart. This does seem pretty necessary for writing tests against absinthe in general, though :)

This is slightly orthoganal to #134 because its per-source

Addresses some if not all of #129

[zachdaniel]

@benwilson512 if you have a few to look at this/let me know what it would take to get this over the line it would be great.

[benwilson512]

I'm 💯 with the need to do this. However I think this requires a major version bump due to the introduction of a new protocol fun.

[benwilson512]

So my only concern here is that this is functionally a breaking change right? Any other 3rd party protocol impl will error now that this is here.

[zachdaniel]

Yeah, absolutely. that is the main reason this was opened as a draft actually. There may be a way around it, which would cause them simply to warn instead of raise, which would be to do something like:

if function_exported?(Dataloader.impl_for(module), :async?, 1) do
  ....
end

But that also adds a concrete performance overhead for other users. To prevent breaking this in the future again, if additional options are needed, we could make it options/1 instead of async?/1 that way if more options are added we don't need a major release.

def options(dataloader) do
  [
    async?: false
  ]
end

[benwilson512]

Also can you rebase off of main to remove the then ?

[zachdaniel]

I think the then/2 is still in master? https://github.com/absinthe-graphql/dataloader/blob/master/test/dataloader/ecto_test.exs#L41

[benwilson512]

Ah apologies it got removed elsewhere. With the release of 1.14 we can probably move supported versions up to ~> 1.12

[zachdaniel]

Yeah, that makes sense! I don't think me changing the matrix in my PR will change what builds for the PR (IIRC it uses main's github.yml file not the PR's github file, to avoid things like exfiltrating secrets). I'm happy to make that change in this PR, but it might make sense as a separate PR.

Do we feel like requiring a major version update is acceptable for this? Or do we want to find a more creative solution?

[zachdaniel]

Hey @benwilson512 so if a major version number is out of the question, how about allowing application configuration to determine async? Its unideal because I'd have to ask my users to put this in their application env when testing with AshGraphql, but would be a potential compromise. i.e Application.get_env(:dataloader, dataloader, async?) (I don't think we have their otp app here to look up the config there?)

[benwilson512]

@zachdaniel OK here is my current thinking. I am going to make a PR that basically just has dataloader accept an async: false option that forces everything to be async false. Then folks can update their Dataloader.new calls to set that option based on repo.in_transaction?. This will be the backwards compatible patch version solution.

THEN we'll go ahead and get this PR across the finish line and this will be the 2.0 version that can handle async on a more granular level depending on the source.

Thoughts?

[zachdaniel]

Sounds like a plan to me :D

[benwilson512]

I think from a code standpoint, I'm gonna go ahead and merge this into master, set the 2.0-pre version, and then checkout v1 and edit the code back to get rid of the protocol impl. That way your code gets attributed properly instead of me copying and pasting 85% of what you wrote.