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.

Sign up for GitHub

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

Jump to bottom

chore(types): Improve TS types for renderHTML and related functions #4605

Merged
merged 9 commits into from
May 2, 2024
Merged

chore(types): Improve TS types for renderHTML and related functions #4605

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6bed705
chore(types): Improve TS types for renderHTML
Tobbe Apr 29, 2024
d4e915b
changeset
Tobbe Apr 29, 2024
e0260b9
Add author and PR to changeset
Tobbe Apr 29, 2024
51a576a
Use RequestOptions instead of Request
Tobbe Apr 29, 2024
3f0ae86
Update changeset text
Tobbe Apr 29, 2024
03d73b5
Separate type import
Tobbe Apr 29, 2024
bf8e98b
Explain isArray check
Tobbe Apr 29, 2024
f8f2933
Add verdaccio/url to changeset
Tobbe Apr 29, 2024
faf5896
Reverting: throw on wrong type
Tobbe May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/itchy-mangos-wink.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@verdaccio/middleware': patch
mbtools marked this conversation as resolved.
Show resolved Hide resolved
'@verdaccio/url': patch
---

Improved TS types for renderHTML() and related functions (by @tobbe in #4605)
18 changes: 14 additions & 4 deletions packages/core/url/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import buildDebug from 'debug';
import type { IncomingHttpHeaders } from 'node:http';
import { URL } from 'url';
import validator from 'validator';

Expand Down Expand Up @@ -31,7 +32,7 @@ export function isHost(url: string = '', options = {}): boolean {
/**
* Detect running protocol (http or https)
*/
export function getWebProtocol(headerProtocol: string | void, protocol: string): string {
export function getWebProtocol(headerProtocol: string | undefined, protocol: string): string {
let returnProtocol;
const [, defaultProtocol] = validProtocols;
// HAProxy variant might return http,http with X-Forwarded-Proto
Expand Down Expand Up @@ -101,7 +102,7 @@ export type RequestOptions = {
/**
* Request headers.
*/
headers: { [key: string]: string };
headers: IncomingHttpHeaders;
remoteAddress?: string;
/**
* Logged username the request, usually after token verification.
Expand All @@ -119,10 +120,19 @@ export function getPublicUrl(url_prefix: string = '', requestOptions: RequestOpt
if (!isHost(host)) {
throw new Error('invalid host');
}
const protoHeader =

const protoHeader: string =
process.env.VERDACCIO_FORWARDED_PROTO?.toLocaleLowerCase() ??
HEADERS.FORWARDED_PROTO.toLowerCase();
const protocol = getWebProtocol(requestOptions.headers[protoHeader], requestOptions.protocol);
const forwardedProtocolHeaderValue = requestOptions.headers[protoHeader];

if (Array.isArray(forwardedProtocolHeaderValue)) {
// This really should never happen - only cookies are allowed to have
// multiple values.
throw new Error('invalid protocol header value');
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider this at least a bit bigger than a chore between minor and patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It should have been patch I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I kind of want to split this PR up into two. First one where I just make this change, and then one where I improve the types. Do you want me to do that?

Copy link
Member

Choose a reason for hiding this comment

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

I would recommend remove this part in order do not block the types part so I can give it some additional thoughts, because I remember this part might be an array, I barely remember

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it. But have to type cast instead otherwise TS complains about the types (and rightfully so).

As soon as this is merged I'll open a new PR where I bring this back again. And I'll try to explain exactly how it changes behavior and in what scenarios.

}

const protocol = getWebProtocol(forwardedProtocolHeaderValue, requestOptions.protocol);
const combinedUrl = combineBaseUrl(protocol, host, url_prefix);
debug('public url by request %o', combinedUrl);
return combinedUrl;
Expand Down
26 changes: 18 additions & 8 deletions packages/middleware/src/middlewares/web/utils/renderHTML.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,55 @@
import buildDebug from 'debug';
import type { Response } from 'express';
import LRU from 'lru-cache';
import path from 'path';
import { URL } from 'url';

import { WEB_TITLE } from '@verdaccio/config';
import { HEADERS } from '@verdaccio/core';
import { ConfigYaml, TemplateUIOptions } from '@verdaccio/types';
import { isURLhasValidProtocol } from '@verdaccio/url';
import { getPublicUrl } from '@verdaccio/url';
import type { RequestOptions } from '@verdaccio/url';
import { getPublicUrl, isURLhasValidProtocol } from '@verdaccio/url';

import type { Manifest } from './manifest';
import renderTemplate from './template';
import type { WebpackManifest } from './template';
import { hasLogin, validatePrimaryColor } from './web-utils';

const DEFAULT_LANGUAGE = 'es-US';
const cache = new LRU({ max: 500, ttl: 1000 * 60 * 60 });

const debug = buildDebug('verdaccio:web:render');

const defaultManifestFiles = {
const defaultManifestFiles: Manifest = {
js: ['runtime.js', 'vendors.js', 'main.js'],
ico: 'favicon.ico',
css: [],
};

export function resolveLogo(config: ConfigYaml, req) {
export function resolveLogo(config: ConfigYaml, requestOptions: RequestOptions) {
if (typeof config?.web?.logo !== 'string') {
return '';
}
const isLocalFile = config?.web?.logo && !isURLhasValidProtocol(config?.web?.logo);

if (isLocalFile) {
return `${getPublicUrl(config?.url_prefix, req)}-/static/${path.basename(config?.web?.logo)}`;
return `${getPublicUrl(config?.url_prefix, requestOptions)}-/static/${path.basename(config?.web?.logo)}`;
} else if (isURLhasValidProtocol(config?.web?.logo)) {
return config?.web?.logo;
} else {
return '';
}
}

export default function renderHTML(config: ConfigYaml, manifest, manifestFiles, req, res) {
export default function renderHTML(
config: ConfigYaml,
manifest: WebpackManifest,
manifestFiles: Manifest | null | undefined,
requestOptions: RequestOptions,
res: Response
) {
const { url_prefix } = config;
const base = getPublicUrl(config?.url_prefix, req);
const base = getPublicUrl(config?.url_prefix, requestOptions);
const basename = new URL(base).pathname;
const language = config?.i18n?.web ?? DEFAULT_LANGUAGE;
const hideDeprecatedVersions = config?.web?.hideDeprecatedVersions ?? false;
Expand All @@ -51,7 +61,7 @@ export default function renderHTML(config: ConfigYaml, manifest, manifestFiles,
const title = config?.web?.title ?? WEB_TITLE;
const login = hasLogin(config);
const scope = config?.web?.scope ?? '';
const logo = resolveLogo(config, req);
const logo = resolveLogo(config, requestOptions);
const pkgManagers = config?.web?.pkgManagers ?? ['yarn', 'pnpm', 'npm'];
const version = res.locals.app_version ?? '';
const flags = {
Expand Down