fix: Improve dependency injection order

[rubenferreira97]

Description

The current order of dependency injection, where dependencies are injected from bottom to top, feels unnatural and counterintuitive. If there is another pattern to prevent this I think it should be documented.

Steps To Reproduce

// routes/_middleware.dart
Handler middleware(Handler handler) {

  /* Gives exception
  return handler
      .use(provider<int>((_) => 1))
      .use(provider<String>((context) => context.read<int>().toString()));
  */

  // Runs fine
  return handler
      .use(provider<String>((context) => context.read<int>().toString()))
      .use(provider<int>((_) => 1));
}

// routes/index.dart
Response onRequest(RequestContext context) {
  return Response(body: context.read<String>()); // Should print '1'
}

Expected Behavior Dependency injection should follow a sequential order for proper execution. The following code should work without issues:

Handler middleware(Handler handler) {
  return handler
      .use(provider<int>((_) => 1))
      .use(provider<String>((context) => context.read<int>().toString()));
}

Response onRequest(RequestContext context) {
  return Response(body: context.read<String>()); // Should print '1'
}

And the following IMO should fail:

Handler middleware(Handler handler) {
  return handler
      .use(provider<String>((context) => context.read<int>().toString()))
      .use(provider<int>((_) => 1));
}

Response onRequest(RequestContext context) {
  return Response(body: context.read<String>()); // Should print '1'
}

[quentin7b]

I agree !

I lost some time on this... In the meantime it might be a good idea to precise this behavior in the documentation ?

[alestiago]

Hi @quentin7b , I believe this change would probably affect significantly how users structure their code, so the team might need some time to evaluate it further. However, adding more documentation around this behaviour sounds like a good idea to me. If you're interested in contributing let us know and we'll be happy to review your Pull Request.

[quentin7b]

Hi ! Of course I can add a specific section in the https://dartfrog.vgv.dev/docs/basics/dependency-injection page that describes this behavior and mention this issue ? What do you think about this ?

[alestiago]

Hi @quentin7b , that sounds good to me. It will be great if you can describe the current behaviour and include examples that show and build the readers intuition around it. Give it a try and we'll happily review it! 🙌

[rubenferreira97]

Even if documentation is added (which is an improvement), I still think it would be nice to address this. Yes, it is a breaking change, but I believe this is a kind of issue for newcomers.

[alestiago]

@rubenferreira97 thanks for hopping in! I agree that documentation might not be the only solution. As it is a breaking change, we would need the team to sit down and evaluate this properly before committing to it. Right now the team has limited bandwidth and we are not foreseeing to prioritise this work within the next couple of weeks. If there's more community engagement in this issue (👍 reactions) we will probably adjust this prioritisation.

If you're eager to have this in and would like to draft a Pull Request so the team can easily evaluate what this change intakes, go for it! We will happily review it and appreciate it 💙

[quentin7b]

I've made a first version in #1218 if you want to give it an eye :)

[tomarra]

Hi everyone 👋 and thanks for participating in this issue!

Thanks @quentin7b for updating the documentation via #1218, which you can read in the Dart Frog documentation.

We've discussed this and the team also thinks that changing the dependency order would make it more intuitive. At the time of initial implementation we were sticking with it as it is also the way that shelf works.

Moving forward, if we went in and made this change and someone updates their dependencies without looking at the changelog (let's be honest, we have all been guilty of that at some point) it's possible that bugs would be introduced unknowingly. Given that, we think there might be a clear path that can be taken here and we are looking for some feedback on the matter.

We're evaluating the idea of providing a new API with the dependency injection ordering fix. This would be more work and we would have to then depreciate the current API but it would become a clearly marked change for anyone looking at the framework during their implementation. Since this would then not be a breaking change at all, it could be easily added into a minor release version instead of having to bump up the package to v2.0.0 overall and hope that developers and teams are actually watching their dependency changelogs.

As usual, we are open to input on this and looking forward to hear what you all have to say. As usual we would also be totally open to someone helping with the implementation as well and we could have the code owners help move the proposed changes through the PR process.

[rubenferreira97]

In my opinion, this is a breaking change. So, if dart_frog follows semver, I would bump this version to 2.0.0 when/if this lands to notify users that their code will break. I also wouldn't mind waiting for more API changes (if there are any). But, as I already mentioned, I think this should be addressed in the future.

[alestiago]

Thanks for dropping your two cents @rubenferreira97 💙 . The plan, so far, is to address this in the future 🙌