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

[PROPOSAL] Add pre-registry #2339

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[PROPOSAL] Add pre-registry #2339

wants to merge 4 commits into from

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Dec 14, 2021

Implementation of #2340

@ahopkins ahopkins changed the title Add pre-registry [PROPOSAL] Add pre-registry Dec 14, 2021
@prryplatypus
Copy link
Member

I keep thinking about this PR and really not sure if I’m for or against this change.

This change is good in the sense that it gives people more flexibility but, on the other hand, I think it can contribute to introducing code smells/bad practices like unused imports, as people will still have to import the module at some point (just like they have to do now) in order for the blueprint to be defined and consequently, registered.

Only real difference as far as I can tell is that it won’t be necessary to call “app.blueprint“, which I’m not sure it’s worth the hassle and added complexity in the code.

Let me know your views on this!

@ahopkins
Copy link
Member Author

There are two things that are driving this:

  1. the divide between app and blueprint. Ideally nothing would be attached to an app, and it is a pass thru. It is not intuitive that they behave differently.
  2. it would localize all logic related to a BP.

The import issues is one that I believe is best solved with auto discovery, which is something that cleans up the import issue even today without this change. The import problem ultimately stems from the usage of global variables. At worst I see this as a wash as that is concerned. Neither solving nor exacerbating that problem.

@ahopkins
Copy link
Member Author

ahopkins commented Jan 16, 2022

The more thought I put into this, the more I think this might pave the way for a real nice path for plugin developers. Most plugins require creating an extraneous an unnecessary instance. Take my own sanic-jwt for example:

from sanic_jwt import Initialize

app = Sanic(__name__)
Initialize(app, ...)

This is not a great API, and it gets away from the idea that the Config object controls your application. Instead, these plugins could attach using the pre-registry:

bp = Blueprint("ThirdPartyPlugin")
bp.pre_register(_default)


@bp.get("/")
async def handler(request: Request):
    return json({"foo": "bar"})

Maybe to get around the import issue we add a top-level API for it:

from path.to.somewhere import thing

app.load(
    thing,
    "path.as.string",
)

There just seems like a real nice potential for creating highly composable applications with easily shareable blueprints.

The other thing that I would like to push into this PR upon further consideration is to more heavily push everything into BPs to begin with. This means that creating an application instance would implicitly create a BP. Anything attached with app.route, app.middleware, etc would be passed thru to the implicit BP.

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

Successfully merging this pull request may close these issues.

None yet

2 participants