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(core): add app.addSetup for asynchronous configuration steps #2510

Closed
wants to merge 2 commits into from

Conversation

vonagam
Copy link
Member

@vonagam vonagam commented Dec 13, 2021

Connected to #2500 and contains small fixes from #1611.

Adds app.addSetup() which allows adding asynchronous configuration/initialization steps.
Those will run with services setups during app.setup() call.

Services use this method to add their own setup calls to queue,
so one can add a step that runs before or after a specific service setup.

This is for cases when you don't have an appropriate service to implement setup method in.

Will add tests if that sounds as a reasonable addition.

@marshallswain
Copy link
Member

I like this idea. It nicely solves the concerns you had in #2500 (comment). Looks good to me.

@vonagam
Copy link
Member Author

vonagam commented Apr 4, 2022

I will close this one since recent hooks for setup/teardown from #2585 should do the trick.

But will note that setting this._isSetup = true should be done before services begin to register, not after, otherwise dynamically registered services from some setup will not get their own setup called. (It was fixed in #1611, but then #2585 put it back because it was based on a code before the fix.)

@vonagam vonagam closed this Apr 4, 2022
@daffl
Copy link
Member

daffl commented Apr 5, 2022

Ah yes, I meant to get back to ask if that new one may not cover everything but I'm hoping it will suffice. Ugh, I thought I kept your _isSetup change in there because that made sense. Will have to revert that part.

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

Successfully merging this pull request may close these issues.

None yet

3 participants