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

feat(dart_frog): allow access of shelf.Message.context #1262

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sun-jiao
Copy link

@sun-jiao sun-jiao commented Jan 28, 2024

Status

READY

Description

Allow access of shelf.Message.context, which is used by some shelf middleware and shelf handlers.

closes #773:

Handler middleware(Handler handler) {
  return (context) {
    final request = context.request;
    print((request.shelfContext['shelf_router/params'] as Map<String, String>?)?['test']); // no longer needs to use `fromShelfHandler` and `toShelfHandler`.
    return handler(context);
  };
}

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@sun-jiao sun-jiao requested a review from a team as a code owner January 28, 2024 14:27
@wolfenrain wolfenrain changed the title fix(dart_frog): allow access of shelf.Message.context feat(dart_frog): allow access of shelf.Message.context Jan 30, 2024
@tomarra
Copy link
Contributor

tomarra commented Jan 30, 2024

Hi @sun-jiao 👋 thanks for opening this PR. It looks like there is an error in the CI pipeline due to missing tests. Can you please take a look and get this passing? Then I can work with the team to get feedback/reviews. Thanks in advance!

@sun-jiao
Copy link
Author

@tomarra Testcases are already added.

@sun-jiao
Copy link
Author

sun-jiao commented Feb 2, 2024

@tomarra any updates?

@wolfenrain
Copy link
Member

Hi thanks for the PR! I had a quick look and I think we should not expose the shelf context like this. Dart Frog tries to stay agnostic to it's underlying framework where it can and exposing this context would counter that.

Based on the example you gave I presume you wanted to have access to the route params in the middleware. I think we should rather focus on that use-case than exposing all of the context as a map. We could potentially expose those parameters in the the same way that we do with the onRequest method.

Handler middleware(Handler handler) {
  return (context, String routeParam1, String routeParam2) {
  
    // ... code ...
    
    return handler(context);
  }
}

This would require us to tweak how we build the routing pipeline but it should be doable and would bring the API more inline with the rest of Dart Frog. Any thoughts on that?

cc: @felangel

@sun-jiao
Copy link
Author

sun-jiao commented Feb 3, 2024

@wolfenrain No, currently I am not concerned with the router's middleware parameters. I'm developing a library (in fact, based on another shelf library) which provides session persistence for shelf. And I want to add support for frog. While it saves the session in shelf's request.context. This is the library: https://github.com/sun-jiao/session_shelf/tree/dart_frog.

@sun-jiao
Copy link
Author

@wolfenrain Hi, any updates?

@sun-jiao
Copy link
Author

sun-jiao commented Mar 10, 2024

Hello, any updates? Or is there any more elegant way to implement the same function without exposing shelf context? @wolfenrain

Sessions are put in shelf.Context in my code: https://github.com/sun-jiao/session_shelf/blob/dart_frog/lib/middleware/cookies_middleware.dart#L67C22-L67C34

In fact, I just did a search before opening this PR to confirm if there was a duplicate, and found issue #773 occasionally.

@wolfenrain
Copy link
Member

Hello, any updates? Or is there any more elegant way to implement the same function without exposing shelf context? @wolfenrain

Sessions are put in shelf.Context in my code: sun-jiao/session_shelf@dart_frog/lib/middleware/cookies_middleware.dart#L67C22-L67C34

In fact, I just did a search before opening this PR to confirm if there was a duplicate, and found issue #773 occasionally.

Hi! Sorry for the late reply I'll be looking into what you linked later this week, preferably we would not want to expose that kind of shell stuff so I'll have a look at your use-case to see if we can come up with a different approach, otherwise we can always expose it as a @visibleForTesting so you could use it without us promoting for normal use too much

@sun-jiao
Copy link
Author

@wolfenrain Thanks.

@alestiago
Copy link
Contributor

@sun-jiao we're also have a Dart Plugin System on the works that would probably help you improve the developer experience for the users of your tool that rely on Dart Frog. Keep an eye on the next updates 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Community
Development

Successfully merging this pull request may close these issues.

feat: Allow routing parameters in middleware
4 participants