Use http server over local domain socket instead of json-socket #3
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,126 +1,89 @@ | ||
| import { | ||
| PackageResponse, | ||
| RequestPackageOptions, | ||
| WantedDependency, | ||
| } from '@pnpm/package-requester' | ||
|
|
||
| import got = require('got') | ||
| import pLimit = require('p-limit') | ||
| import {StoreController} from 'package-store' | ||
| import uuid = require('uuid') | ||
|
|
||
| export default function ( | ||
| initOpts: { | ||
| path?: string, | ||
| port?: number, | ||
| hostname?: string, | ||
| }, | ||
| ): Promise<StoreController> { | ||
| let remotePrefix: string | ||
| if (initOpts.path) { | ||
| remotePrefix = `http://unix:${initOpts.path}:` | ||
| } else { | ||
| remotePrefix = `http://${initOpts.hostname}:${initOpts.port}` | ||
|
There was a problem hiding this comment. I think it would be fine to pass in the formed path, these manipulations can be done by pnpm |
||
| } | ||
| const limitedRetryFetch = retryFetch.bind(null, pLimit(100)) | ||
|
There was a problem hiding this comment. I'd make the concurrency configurable |
||
|
|
||
| return new Promise((resolve, reject) => { | ||
| resolve({ | ||
| close: async () => { return }, | ||
| prune: async () => { | ||
| await limitedRetryFetch(`${remotePrefix}/prune`, {}) | ||
| }, | ||
| requestPackage: requestPackage.bind(null, remotePrefix, limitedRetryFetch), | ||
| saveState: async () => { | ||
| await limitedRetryFetch(`${remotePrefix}/saveState`, {}) | ||
| }, | ||
| updateConnections: async (prefix: string, opts: {addDependencies: string[], removeDependencies: string[], prune: boolean}) => { | ||
| await limitedRetryFetch(`${remotePrefix}/updateConnections`, { | ||
| opts, | ||
| prefix, | ||
| }) | ||
| }, | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| function retryFetch (limit: (fn: () => PromiseLike<object>) => Promise<object>, url: string, body: object): Promise<object> { // tslint:disable-line | ||
| return limit(async () => { | ||
| const response = await got(url, { | ||
| body: JSON.stringify(body), | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| method: 'POST', | ||
| }) | ||
|
|
||
| return JSON.parse(response.body) | ||
| }).catch((e) => { | ||
|
There was a problem hiding this comment. you can use async/await syntax here with a regular try/catch also, I think the try/catch should be inside the function and cover just the |
||
| if (!e.message.startsWith('Error: connect ECONNRESET') && !e.message.startsWith('Error: connect ECONNREFUSED')) { | ||
| throw e | ||
| } | ||
| return retryFetch(limit, url, body) | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. and is there a way to tell got to retry as many times as it's needed? There was a problem hiding this comment. yes, by default it reties 2 times |
||
| }) | ||
| } | ||
|
|
||
| function requestPackage ( | ||
| remotePrefix: string, | ||
| limitedRetryFetch: (url: string, body: object) => any, // tslint:disable-line | ||
| wantedDependency: WantedDependency, | ||
| options: RequestPackageOptions, | ||
| ): Promise<PackageResponse> { | ||
| const msgId = uuid.v4() | ||
|
|
||
| return limitedRetryFetch(`${remotePrefix}/requestPackage`, { | ||
| msgId, | ||
| options, | ||
| wantedDependency, | ||
| }) | ||
| .then((packageResponse: PackageResponse) => { | ||
| const fetchingManifest = limitedRetryFetch(`${remotePrefix}/manifestResponse`, { | ||
| msgId, | ||
| }) | ||
| const fetchingFiles = limitedRetryFetch(`${remotePrefix}/packageFilesResponse`, { | ||
| msgId, | ||
| }) | ||
| return Object.assign(packageResponse, { | ||
| fetchingFiles, | ||
| fetchingManifest, | ||
| finishing: Promise.all([fetchingManifest, fetchingFiles]).then(() => undefined), | ||
| }) | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,91 +1,84 @@ | ||
| import http = require('http') | ||
| import {IncomingMessage, Server, ServerResponse} from 'http' | ||
| import {StoreController} from 'package-store' | ||
|
|
||
| export default function ( | ||
| store: StoreController, | ||
| opts: { | ||
| path?: string, | ||
| port?: number, | ||
| hostname?: string, | ||
| }, | ||
| ) { | ||
| const manifestPromises = {} | ||
| const filesPromises = {} | ||
|
|
||
| const server = http.createServer(async (req: IncomingMessage, res: ServerResponse) => { | ||
| if (req.method !== 'POST') { | ||
| res.statusCode = 503 | ||
| res.end(JSON.stringify(`Only POST is allowed, received ${req.method}`)) | ||
| return | ||
| } | ||
|
|
||
| let body: any = '' // tslint:disable-line | ||
| req.on('data', (data) => { | ||
| body += data | ||
| }) | ||
| req.on('end', async () => { | ||
| try { | ||
| if (body.length > 0) { | ||
| body = JSON.parse(body) | ||
| } else { | ||
| body = {} | ||
| } | ||
|
|
||
| switch (req.url) { | ||
|
There was a problem hiding this comment. the switch statement should be probably executed early, because if an unknown route is requested, there's no need to read the data There was a problem hiding this comment. The switch cannot be executed early because we don't have the body until the request has been received, and I don't see any reason to add complexity to wait for the body since we always need the body or it is empty so it doesn't add overhead. There was a problem hiding this comment. isn't the URL known from the very start? The URL can be validated before collecting the body There was a problem hiding this comment. and once we validate the URL? We've to wait for the body... so I am not really sure why we should validate the URL first |
||
| case '/requestPackage': | ||
| const pkgResponse = await store.requestPackage(body.wantedDependency, body.options) | ||
| if (!pkgResponse.isLocal) { | ||
| manifestPromises[body.msgId] = pkgResponse.fetchingManifest | ||
| filesPromises[body.msgId] = pkgResponse.fetchingFiles | ||
| } | ||
| res.end(JSON.stringify(pkgResponse)) | ||
| break | ||
| case '/packageFilesResponse': | ||
| const filesResponse = await filesPromises[body.msgId] | ||
| delete filesPromises[body.msgId] | ||
| res.end(JSON.stringify(filesResponse)) | ||
| break | ||
| case '/manifestResponse': | ||
| const manifestResponse = await manifestPromises[body.msgId] | ||
| delete manifestPromises[body.msgId] | ||
| res.end(JSON.stringify(manifestResponse)) | ||
| break | ||
| case '/updateConnections': | ||
| await store.updateConnections(body.prefix, body.opts) | ||
| res.end(JSON.stringify('OK')) | ||
| break | ||
| case '/prune': | ||
| await store.prune() | ||
| res.end(JSON.stringify('OK')) | ||
| break | ||
| case '/saveState': | ||
| await store.saveState() | ||
| res.end(JSON.stringify('OK')) | ||
| break | ||
| } | ||
|
There was a problem hiding this comment. if no route was matched, I think a 404 response should be returned |
||
| } catch (e) { | ||
| res.statusCode = 503 | ||
| res.end(JSON.stringify(e.message)) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| let listener: Server; | ||
| if (opts.path) { | ||
| listener = server.listen(opts.path) | ||
| } else { | ||
| listener = server.listen(opts.port, opts.hostname) | ||
| } | ||
|
|
||
| return { | ||
| close: () => listener.close(() => { return }), | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type dependencies of prod dependencies should be also prod dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok