Ensrainbow init #101
Ensrainbow init #101
Conversation
There was a problem hiding this comment.
@djstrong Really amazing work here 🚀 😄 🙌 !! I've added some suggestions here, but all are pretty low priority and can be done in separate future PRs. Please feel welcome to merge this anytime!
|
@djstrong Ah I see the CI is complaining about In this commit: 314f320 I see we made a change to apps/ensrainbow/package.json but there was no accompanying change to apps/ensrainbow/package-lock.json. Easiest fix is to delete |
apps/ensrainbow/README.md
Outdated
|
|
||
| - `PORT`: Server port (default: 3001) | ||
| - `DATA_DIR`: Directory for LevelDB data (default: './data') | ||
| - `NODE_ENV`: Node environment (default: 'production' in Docker) |
There was a problem hiding this comment.
| - `NODE_ENV`: Node environment (default: 'production' in Docker) | |
| - `NODE_ENV`: Node.js environment (default: 'production' in Docker) |
Since we're in "ENS Land" there's multiple interpretations for "Node", therefore suggesting the modification above.
Also, could you please include more info on what the purpose of this environment variable is? I understand it's something for Node.js, but is there a special reason we're mentioning it in our readme file here?
There was a problem hiding this comment.
Actually, it does nothing now, but we can use it e.g. to show more debug logs. I am removing it from the readme.
There was a problem hiding this comment.
@djstrong Hey super updates! Nice work 🙌 Reviewed and shared feedback. Overall I suggest merging this PR and then following up on the comments in separate future PRs.
Thanks!
| @@ -0,0 +1,12 @@ | |||
| import { defineConfig } from 'vitest/config'; | |||
|
|
|||
| export default defineConfig({ | |||
There was a problem hiding this comment.
This is configuration required for generating test coverage reports. We don't have that on the ensnode app, but we have the test:coverage task defined in package.json for the ensrainbow app.
apps/ensrainbow/src/index.ts
Outdated
| app.get('/health', (c: Context) => c.json({ status: 'ok' })); | ||
|
|
||
| // Get count of healable labels | ||
| app.get('/v1/labels/count', async (c: Context) => { |
There was a problem hiding this comment.
Created a separate low priority for us to enhance this in a separate future PR: #121
apps/ensrainbow/src/index.test.ts
Outdated
| const response = await fetch('http://localhost:3002/v1/heal/'); | ||
| expect(response.status).toBe(404); // Hono returns 404 for missing parameters | ||
| const text = await response.text(); | ||
| expect(text).toBe('404 Not Found'); // Hono's default 404 response |
There was a problem hiding this comment.
Interesting. Perhaps we should add some comment about thisinside the implementation of this API?
For example, I see this code in the implementation:
if (!labelhash) {
return c.json({ error: 'Missing labelhash parameter' }, 400);
}
That suggested to me that Hono would return exactly that, but according to this test, it doesn't. Not sure if we can configure Hono to handle that differently or if we should just leave Hono the way it is but add some comments in the implementation identifying that the handler won't even be called by Hono if the param is missing.
Advice appreciated.
| import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest'; | ||
| import { app, db } from './index'; | ||
| import { serve } from '@hono/node-server'; | ||
|
|
There was a problem hiding this comment.
I created a low priority story with general ideas on the testing strategy here that we can follow up on later: #123
Co-authored-by: lightwalker.eth <[email protected]>
|
Please let's merge #116 before going live with this PR. |
This PR fixes docker build by using optimised layers and building apps from directly monorepo root directory. Also, automated code style fixes were applied on the existing codebase. Co-authored-by: kwrobel.eth <[email protected]>
#84