Implement http registry #464
Conversation
🦋 Changeset detectedLatest commit: e4c6c85 The changes in this PR will be included in the next version bump. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
| "name": "@hyperlane-xyz/registry", | ||
| "description": "A collection of configs, artifacts, and schemas for Hyperlane", | ||
| "version": "6.11.0", | ||
| "version": "6.12.0-http.2", | ||
| "dependencies": { | ||
| "express": "^4.21.2", |
There was a problem hiding this comment.
wonder if it makes sense to spin this off to a side-package so it doesn't add to the normal registry size
There was a problem hiding this comment.
Putting it in it's own export path like I did for index.fs is a good idea. Not just for bundle bloat but also because express isn't meant to be run in the browser so its inclusion could cause errors.
| @@ -1,5 +1,23 @@ | |||
| # @hyperlane-xyz/registry | |||
|
|
|||
| ## 6.12.0-http.2 | |||
There was a problem hiding this comment.
Why has the changelog changed as part of your PR?
There was a problem hiding this comment.
this was before I figured out how to get yarn link working I needed to publish some beta versions for the monorepo PR to use
| @@ -1,15 +1,17 @@ | |||
| { | |||
| "name": "@hyperlane-xyz/registry", | |||
| "description": "A collection of configs, artifacts, and schemas for Hyperlane", | |||
| "version": "6.11.0", | |||
| "version": "6.12.0-http.2", | |||
There was a problem hiding this comment.
Do you mean to merge this?
| "name": "@hyperlane-xyz/registry", | ||
| "description": "A collection of configs, artifacts, and schemas for Hyperlane", | ||
| "version": "6.11.0", | ||
| "version": "6.12.0-http.2", | ||
| "dependencies": { | ||
| "express": "^4.21.2", |
There was a problem hiding this comment.
Putting it in it's own export path like I did for index.fs is a good idea. Not just for bundle bloat but also because express isn't meant to be run in the browser so its inclusion could cause errors.
| }); | ||
| } | ||
|
|
||
| get type(): RegistryType { |
There was a problem hiding this comment.
Avoid get proxy methods. They have pitfalls. Use simple methods/members instead.
| } | ||
|
|
||
| get type(): RegistryType { | ||
| throw new Error('Method not implemented.'); |
There was a problem hiding this comment.
Why not set a RegistryType?
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| get uri(): string { |
src/registry/HttpServer.ts
Outdated
| this.app.get('/metadata', async (req, res) => { | ||
| try { | ||
| const metadata = await registry.getMetadata(); | ||
| if (!metadata) { |
There was a problem hiding this comment.
Nbd but I don't think getMetadata can ever return a falsy value. Maybe you meant to check that Object.keys().length > 0?
There was a problem hiding this comment.
this was AI generated lol
will clean this up before setting ready for review
src/registry/HttpServer.ts
Outdated
| } | ||
|
|
||
| start(port = process.env.PORT || 3000) { | ||
| return new Promise<void>((resolve, reject) => { |
There was a problem hiding this comment.
Does this method get awaited somewhere? Returning a process for a long-lived job like a server seems odd to me
Description
HttpServerREST API wrapper aroundIRegistryHttpClientRegistrywhich implementsIRegistryfor use in clients like the CLI or warp UIBackward compatibility
Yes
Testing
Manual