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.

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

fix: minor UX fixes for installing extensions #4308

Merged
merged 10 commits into from
Oct 1, 2020
2 changes: 0 additions & 2 deletions Composer/packages/client/src/pages/setting/SettingsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,6 @@ const SettingPage: React.FC<RouteComponentProps> = () => {
useEffect(() => {
if (!projectId && location.pathname.indexOf('/settings/bot/') !== -1) {
navigate('/settings/application');
} else {
navigate(links[0].url);
}
}, [projectId]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@

/** @jsx jsx */
import { jsx, css } from '@emotion/core';
import React from 'react';
import React, { useRef } from 'react';
import formatMessage from 'format-message';
import {
DetailsListLayoutMode,
SelectionMode,
IColumn,
CheckboxVisibility,
ConstrainMode,
Selection,
} from 'office-ui-fabric-react/lib/DetailsList';
import { ScrollablePane } from 'office-ui-fabric-react/lib/ScrollablePane';
import { Sticky } from 'office-ui-fabric-react/lib/Sticky';
Expand Down Expand Up @@ -44,6 +45,13 @@ const noResultsStyles = css`

const ExtensionSearchResults: React.FC<ExtensionSearchResultsProps> = (props) => {
const { results, isSearching, onSelect } = props;
const selection = useRef(
new Selection({
onSelectionChanged: () => {
onSelect(selection.getSelection()[0] as ExtensionSearchResult);
},
})
).current;

const searchColumns: IColumn[] = [
{
Expand Down Expand Up @@ -101,9 +109,9 @@ const ExtensionSearchResults: React.FC<ExtensionSearchResultsProps> = (props) =>
enableShimmer={isSearching}
items={noResultsFound ? [{}] : results}
layoutMode={DetailsListLayoutMode.justified}
selection={selection}
selectionMode={SelectionMode.single}
shimmerLines={8}
onActiveItemChanged={(item) => onSelect(item)}
onRenderDetailsHeader={(headerProps, defaultRender) => {
if (defaultRender) {
return <Sticky>{defaultRender(headerProps)}</Sticky>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ const Extensions: React.FC<RouteComponentProps> = () => {
}, []);

const shownItems = () => {
if (extensions.length === 0) {
// render no installed message
return [{}];
} else if (isUpdating === true) {
if (isUpdating === true) {
// extension is being added, render a shimmer row at end of list
return [...extensions, null];
} else if (typeof isUpdating === 'string') {
// extension is being removed or updated, show shimmer for that row
return extensions.map((e) => (e.id === isUpdating ? null : e));
} else if (extensions.length === 0) {
// render no installed message
return [{}];
} else {
return extensions;
}
Expand All @@ -186,20 +186,25 @@ const Extensions: React.FC<RouteComponentProps> = () => {
selection={selection}
selectionMode={SelectionMode.multiple}
onRenderRow={(rowProps, defaultRender) => {
if (extensions.length === 0) {
return (
<div css={noExtensionsStyles}>
<p>{formatMessage('No extensions installed')}</p>
</div>
);
}
if (rowProps && defaultRender) {
if (isUpdating) {
return defaultRender(rowProps);
}

if (extensions.length === 0) {
return (
<div css={noExtensionsStyles}>
<p>{formatMessage('No extensions installed')}</p>
</div>
);
}

if (defaultRender && rowProps) {
const customStyles: Partial<IDetailsRowStyles> = {
root: {
color: rowProps?.item?.enabled ? undefined : NeutralColors.gray90,
color: rowProps.item?.enabled ? undefined : NeutralColors.gray90,
},
};

return <DetailsRow {...rowProps} styles={customStyles} />;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ const InstallExtensionDialog: React.FC<InstallExtensionDialogProps> = (props) =>
</div>
<DialogFooter>
<DefaultButton onClick={onDismiss}>Cancel</DefaultButton>
<PrimaryButton disabled={false} onClick={onSubmit}>
<PrimaryButton disabled={!selectedExtension} onClick={onSubmit}>
{formatMessage('Add')}
</PrimaryButton>
</DialogFooter>
Expand Down
2 changes: 2 additions & 0 deletions Composer/packages/client/src/recoilModel/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ type ExtensionPublishContribution = {
};

export type ExtensionPageContribution = {
/** plugin id */
id: string;
bundleId: string;
label: string;
icon?: string;
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/utils/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const useLinks = () => {
const pluginPages = extensions.reduce((pages, p) => {
const pagesConfig = p.contributes?.views?.pages;
if (Array.isArray(pagesConfig) && pagesConfig.length > 0) {
pages.push(...pagesConfig);
pages.push(...pagesConfig.map((page) => ({ ...page, id: p.id })));
}
return pages;
}, [] as ExtensionPageContribution[]);
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/utils/pageLinks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export const topLinks = (projectId: string, openedDialogId: string, pluginPages:
if (pluginPages.length > 0) {
pluginPages.forEach((p) => {
links.push({
to: `page/${p.bundleId}`,
to: `page/${p.id}`,
iconName: p.icon ?? 'StatusCircleQuestionMark',
labelName: p.label,
exact: true,
Expand Down
41 changes: 39 additions & 2 deletions Composer/packages/extension/src/manager/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import path from 'path';

import glob from 'globby';
import { readJson, ensureDir } from 'fs-extra';
import { readJson, ensureDir, existsSync } from 'fs-extra';

import { ExtensionContext } from '../extensionContext';
import logger from '../logger';
Expand Down Expand Up @@ -77,6 +77,7 @@ class ExtensionManager {
* Installs a remote extension via NPM
* @param name The name of the extension to install
* @param version The version of the extension to install
* @returns id of installed package
*/
public async installRemote(name: string, version?: string) {
const packageNameAndVersion = version ? `${name}@${version}` : `${name}@latest`;
Expand All @@ -97,6 +98,8 @@ class ExtensionManager {
if (packageJson) {
const extensionPath = path.resolve(this.remoteDir, 'node_modules', name);
this.manifest.updateExtensionConfig(name, getExtensionMetadata(extensionPath, packageJson));

return packageJson.name;
} else {
throw new Error(`Unable to install ${packageNameAndVersion}`);
}
Expand All @@ -108,6 +111,37 @@ class ExtensionManager {
}
}

/**
* Installs a local extension at path
* @param path Path of directory where extension is
*/
public async installLocal(extPath: string) {
try {
const packageJsonPath = path.join(extPath, 'package.json');

if (!existsSync(packageJsonPath)) {
throw new Error(`Extension not found at path: ${extPath}`);
}

const packageJson = await readJson(packageJsonPath);

log('Linking %s', packageJson.name);
await npm('link', '.', {}, { cwd: extPath });

log('Installing %s@local to %s', packageJson.name, this.remoteDir);
await npm('link', packageJson.name, { '--prefix': this.remoteDir }, { cwd: this.remoteDir });

const extensionPath = path.resolve(this.remoteDir, 'node_modules', packageJson.name);
this.manifest.updateExtensionConfig(packageJson.name, getExtensionMetadata(extensionPath, packageJson));

return packageJson.name;
} catch (err) {
log('%s', err.msg ?? err.stderr ?? err);
// eslint-disable-next-line no-console
console.error(err);
}
}

public async load(id: string) {
const metadata = this.manifest.getExtensionConfig(id);
try {
Expand Down Expand Up @@ -174,11 +208,14 @@ class ExtensionManager {
*/
public async search(query: string) {
await this.updateSearchCache();
const normalizedQuery = query.toLowerCase();

const results = Array.from(this.searchCache.values()).filter((result) => {
return (
!this.find(result.id) &&
[result.id, result.description, ...result.keywords].some((target) => target.includes(query))
[result.id, result.description, ...result.keywords].some((target) =>
target.toLowerCase().includes(normalizedQuery)
)
);
});

Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/extension/src/utils/npm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ type NpmOutput = {
stderr: string;
code: number;
};
type NpmCommand = 'install' | 'uninstall' | 'search';
type NpmCommand = 'install' | 'uninstall' | 'search' | 'link';
type NpmOptions = {
[key: string]: string;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,21 +166,40 @@ describe('adding an extension', () => {
expect(ExtensionManager.installRemote).toHaveBeenCalledWith(id, 'some-version');
});

it('loads the extension', async () => {
await ExtensionsController.addExtension({ body: { id } } as Request, res);
describe('installed successfully', () => {
beforeEach(() => {
(ExtensionManager.installRemote as jest.Mock).mockResolvedValue(id);
});

it('loads the extension', async () => {
await ExtensionsController.addExtension({ body: { id } } as Request, res);

expect(ExtensionManager.load).toHaveBeenCalledWith(id);
});

expect(ExtensionManager.load).toHaveBeenCalledWith(id);
it('returns the extension', async () => {
(ExtensionManager.find as jest.Mock).mockReturnValue(mockExtension1);
await ExtensionsController.addExtension({ body: { id } } as Request, res);

expect(ExtensionManager.find).toHaveBeenCalledWith(id);
expect(res.json).toHaveBeenCalledWith({
...mockExtension1,
bundles: undefined,
path: undefined,
});
});
});

it('returns the extension', async () => {
(ExtensionManager.find as jest.Mock).mockReturnValue(mockExtension1);
await ExtensionsController.addExtension({ body: { id } } as Request, res);
describe('install fails', () => {
beforeEach(() => {
(ExtensionManager.installRemote as jest.Mock).mockResolvedValue(undefined);
});

expect(ExtensionManager.find).toHaveBeenCalledWith(id);
expect(res.json).toHaveBeenCalledWith({
...mockExtension1,
bundles: undefined,
path: undefined,
it('returns an error', async () => {
await ExtensionsController.addExtension({ body: { id } } as Request, res);

expect(res.status).toHaveBeenCalledWith(500);
expect(res.json).toHaveBeenCalledWith({ error: expect.any(String) });
});
});
});
Expand Down
14 changes: 10 additions & 4 deletions Composer/packages/server/src/controllers/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface AddExtensionRequest extends Request {
body: {
id?: string;
version?: string;
path?: string;
};
}

Expand Down Expand Up @@ -60,10 +61,15 @@ export async function addExtension(req: AddExtensionRequest, res: Response) {
return;
}

await ExtensionManager.installRemote(id, version);
await ExtensionManager.load(id);
const extension = ExtensionManager.find(id);
res.json(presentExtension(extension));
const extensionId = await ExtensionManager.installRemote(id, version);

if (extensionId) {
await ExtensionManager.load(extensionId);
const extension = ExtensionManager.find(extensionId);
res.json(presentExtension(extension));
} else {
res.status(500).json({ error: 'Unable to install extension.' });
}
}

export async function toggleExtension(req: ToggleExtensionRequest, res: Response) {
Expand Down