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: onReady plugin hook #210

Merged
merged 2 commits into from
Feb 12, 2025
Merged

feat: onReady plugin hook #210

merged 2 commits into from
Feb 12, 2025

Conversation

tpluscode
Copy link
Contributor

No description provided.

@tpluscode tpluscode requested a review from giacomociti January 21, 2025 15:00
Copy link

changeset-bot bot commented Jan 21, 2025

🦋 Changeset detected

Latest commit: e82e9b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@kopflos-cms/core Patch
kopflos Patch
example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tpluscode
Copy link
Contributor Author

I'm not sure about this. I added a new onReady plugin hook which fires when the app started listening.

In principle, I could have used onStart but there are no dependencies between plugins - all run concurrently. Maybe adding a way to wait for another plugin would be a better solution?

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.10%. Comparing base (1dac8dc) to head (e82e9b5).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   94.09%   94.10%   +0.01%     
==========================================
  Files          59       59              
  Lines        2862     2868       +6     
  Branches      338      340       +2     
==========================================
+ Hits         2693     2699       +6     
  Misses        167      167              
  Partials        2        2              
Flag Coverage Δ
@kopflos-cms/core 96.47% <100.00%> (+0.02%) ⬆️
@kopflos-cms/express 86.97% <83.33%> (-0.02%) ⬇️
@kopflos-cms/hydra 89.31% <83.33%> (-0.02%) ⬇️
@kopflos-cms/plugin-deploy-resources 59.62% <83.33%> (+0.14%) ⬆️
@kopflos-cms/serve-file 100.00% <ø> (ø)
@kopflos-cms/shacl 67.19% <ø> (ø)
@kopflos-cms/vite 66.49% <ø> (ø)
@kopflos-labs/handlebars 77.51% <ø> (ø)
@kopflos-labs/html-template 83.93% <ø> (ø)
sparql-path-parser 95.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tpluscode tpluscode merged commit 6b3f86a into master Feb 12, 2025
68 checks passed
@tpluscode tpluscode deleted the plugin-on-ready branch February 12, 2025 09:28
@@ -148,6 +150,10 @@ export default class Impl implements Kopflos {
this.start = onetime(async function (this: Impl) {
await Promise.all(this.plugins.map(plugin => plugin.onStart?.()))
}).bind(this)

this.ready = onetime(async function (this: Impl) {
await Promise.all(this.plugins.map(plugin => plugin.onReady?.()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to await the promises here? is the server really ready to serve requests until the plugins finish their onReady task?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, technically, this await is irrelevant here because it's not awaited when the CLI starts:

const server = app.listen(port, host, () => {
log.info(`Server running on ${port}. API URL: ${config.baseIri}`)
instance.ready()
})

And yes, the server should be able to serve requests. Of course some may still depend on the code in onReady hooks...

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.

2 participants