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

[EPM] rewrite relative image paths #49637

Merged
merged 2 commits into from
Oct 30, 2019
Merged

[EPM] rewrite relative image paths #49637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23bee6e
rewrite relative image paths
neptunian Oct 28, 2019
7017456
remove EuiImage, use transformImageUri, clean up
neptunian Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions x-pack/legacy/plugins/integrations_manager/public/hooks/use_links.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@

import { generatePath } from 'react-router-dom';
import { PLUGIN } from '../../common/constants';
import { API_ROOT } from '../../common/routes';
import { getFilePath, getInfoPath } from '../../common/routes';
import { patterns } from '../routes';
import { useCore } from '.';
import { removeRelativePath } from '../utils/remove_relative_path';
import { DetailViewPanelName } from '..';

// TODO: get this from server/packages/handlers.ts (move elsewhere?)
// seems like part of the name@version change
interface DetailParams {
Expand All @@ -19,6 +19,9 @@ interface DetailParams {
panel?: DetailViewPanelName;
}

const removeRelativePath = (relativePath: string): string =>
new URL(relativePath, 'http://example.com').pathname;

function addBasePath(path: string) {
const { http } = useCore();
return http.basePath.prepend(path);
Expand All @@ -32,10 +35,20 @@ function appRoot(path: string) {
export function useLinks() {
return {
toAssets: (path: string) => addBasePath(`/plugins/${PLUGIN.ID}/assets/${path}`),
toImage: (path: string) => addBasePath(`${API_ROOT}${path}`),
toRelativeImage: ({ path, pkg }: { path: string; pkg: string }) => {
toImage: (path: string) => addBasePath(getFilePath(path)),
toRelativeImage: ({
path,
packageName,
version,
}: {
path: string;
packageName: string;
version: string;
}) => {
const imagePath = removeRelativePath(path);
return addBasePath(`${API_ROOT}/package/${pkg}/${imagePath}`);
const pkgkey = `${packageName}-${version}`;
const filePath = `${getInfoPath(pkgkey)}/${imagePath}`;
return addBasePath(filePath);
},
toListView: () => appRoot(patterns.LIST_VIEW),
toDetailView: ({ name, version, panel }: DetailParams) => {
Expand Down
21 changes: 0 additions & 21 deletions x-pack/legacy/plugins/integrations_manager/public/screens/detail/markdown_renderers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@ import {
EuiTableRow,
EuiTableRowCell,
EuiLink,
EuiImage,
} from '@elastic/eui';
import React from 'react';
import { useLinks } from '../../hooks';

/** prevents links to the new pages from accessing `window.opener` */
const REL_NOOPENER = 'noopener';
Expand All @@ -25,25 +23,6 @@ const REL_NOFOLLOW = 'nofollow';
/** prevents the browser from sending the current address as referrer via the Referer HTTP header */
const REL_NOREFERRER = 'noreferrer';

export const WrappedEuiImage = ({
Copy link
Contributor Author

@neptunian neptunian Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We decided to not use EuiImage here as we don't need Figure and Figcaption and they are causing invalid html warnings due to being wrapped in paragraph tags.

alt,
src,
title,
packageName,
version,
}: {
alt: string;
src: string;
title: string;
packageName: string;
version: string;
}) => {
const { toRelativeImage } = useLinks();
const pkg = `${packageName}-${version}`;
const fullSrc = toRelativeImage({ pkg, path: src });
return <EuiImage url={fullSrc} alt={alt} size="original" caption={title} />;
};

export const markdownRenderers = {
root: ({ children }: { children: React.ReactNode[] }) => (
<EuiText grow={true}>{children}</EuiText>
Expand Down
6 changes: 2 additions & 4 deletions x-pack/legacy/plugins/integrations_manager/public/screens/detail/overview_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { Fragment } from 'react';
import { EuiSpacer, EuiText } from '@elastic/eui';
import { EuiSpacer } from '@elastic/eui';
import { PackageInfo } from '../../../common/types';
import { Screenshots } from './screenshots';
import { Readme } from './readme';
Expand All @@ -13,9 +13,7 @@ export function OverviewPanel(props: PackageInfo) {
const { screenshots, readme, name, version } = props;
return (
<Fragment>
<EuiText>
{readme && <Readme readmePath={readme} packageName={name} version={version} />}
</EuiText>
{readme && <Readme readmePath={readme} packageName={name} version={version} />}
<EuiSpacer size="xl" />
{screenshots && <Screenshots images={screenshots} />}
</Fragment>
Expand Down
23 changes: 15 additions & 8 deletions x-pack/legacy/plugins/integrations_manager/public/screens/detail/readme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import React, { useEffect, useState, Fragment } from 'react';
import { EuiLoadingContent, EuiText } from '@elastic/eui';
import ReactMarkdown from 'react-markdown';
import { getFileByPath } from '../../data';
import { markdownRenderers, WrappedEuiImage } from './markdown_renderers';
import { markdownRenderers } from './markdown_renderers';
import { useLinks } from '../../hooks';

export function Readme({
readmePath,
Expand All @@ -20,6 +21,17 @@ export function Readme({
}) {
const [markdown, setMarkdown] = useState<string | undefined>(undefined);

const handleImageUri = React.useCallback(
(uri: string) => {
const { toRelativeImage } = useLinks();
const isRelative =
uri.indexOf('http://') === 0 || uri.indexOf('https://') === 0 ? false : true;
Copy link
Contributor

@jfsiii jfsiii Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You can also use this to tell if it starts with http: or https:

Suggested change
uri.indexOf('http://') === 0 || uri.indexOf('https://') === 0 ? false : true;
/^http(s?):/.test(uri)

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 was trying to avoid using a regex here, unless you see a case the condition is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't a missing case. Why did you want to avoid a regex? Legability? Something else?

I understand that regexes aren't obvious in many cases, but I think this one is pretty straightforward. Less subjectively, it has less statements and concepts than the multiple indexOfs.

startsWith is another option which makes the intent clear without using a regex

uri.startsWith('http://') || uri.startsWith('https://') ? false : true;

I'm pretty sure corejs will polyfill this for IE, but we would want to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legibility, but they are often less efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, legibility is the only one we should be concerned with for now. I haven't profiled them, but every approach we discussed is likely to do millions of operations per second (e.g. fractions of a millisecond). The difference between 0.000033 ms and .000017 ms doesn't matter to our users or computers. Also, what's slower today can be faster tomorrow when the engine adds an optimization for it.

I think .startsWith is a great approach, but we don't need to resolve this before shipping. It's approved so feel free to merge whenever you wish.

const fullUri = isRelative ? toRelativeImage({ packageName, version, path: uri }) : uri;
return fullUri;
},
[packageName, version]
);

useEffect(() => {
getFileByPath(readmePath).then(res => {
setMarkdown(res);
Expand All @@ -29,14 +41,9 @@ export function Readme({
return (
<Fragment>
{markdown !== undefined ? (
// pass down package name and version props to the image renderer to create image path
<ReactMarkdown
renderers={{
image: ({ ...props }) => (
<WrappedEuiImage {...props} packageName={packageName} version={version} />
),
...markdownRenderers,
}}
transformImageUri={handleImageUri}
renderers={markdownRenderers}
source={markdown}
/>
) : (
Expand Down
13 changes: 0 additions & 13 deletions x-pack/legacy/plugins/integrations_manager/public/utils/remove_relative_path.ts
View file
Open in desktop

This file was deleted.