Skip to content

fix(compass-aggregations): remove ace editor snippet completion, complete operator snippets on our end COMPASS-5683 #3048

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

Merged
merged 2 commits into from
May 3, 2022
Merged

fix(compass-aggregations): remove ace editor snippet completion, complete operator snippets on our end COMPASS-5683 #3048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71bae99
avoid ace editor snippet completion, complete on our end, remove un-n…
Anemy May 3, 2022
a8fdb7e
improve naming on token replacing function, remove snippet on model
Anemy May 3, 2022
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
2 changes: 0 additions & 2 deletions ...ass-aggregations/src/components/pipeline-builder-workspace/pipeline-builder-workspace.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ class PipelineWorkspace extends PureComponent {
sourceName={this.props.sourceName}
stage={stage.stage}
stageOperator={stage.stageOperator}
snippet={stage.snippet}
error={stage.error}
syntaxError={stage.syntaxError}
isValid={stage.isValid}
Expand All @@ -115,7 +114,6 @@ class PipelineWorkspace extends PureComponent {
isExpanded={stage.isExpanded}
isCommenting={this.props.isCommenting}
isAutoPreviewing={this.props.isAutoPreviewing}
fromStageOperators={stage.fromStageOperators || false}
previewDocuments={stage.previewDocuments}
runStage={this.props.runStage}
openLink={this.props.openLink}
Expand Down
4 changes: 0 additions & 4 deletions ...ggregations/src/components/pipeline-builder-workspace/pipeline-builder-workspace.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ const PIPELINE_1 = [
'syntaxError': null,
'error': null,
'projections': [],
'snippet': '/**\n * query: The query in MQL.\n */\n{\n ${1:query}\n}',
'fromStageOperators': false,
'executor': {
'$match': {
'x': 1
Expand All @@ -39,8 +37,6 @@ const PIPELINE_1 = [
'syntaxError': null,
'error': null,
'projections': [],
'snippet': '/**\n * Provide the number of documents to limit.\n */\n${1:number}',
'fromStageOperators': false,
'executor': {
'$limit': 3
}
Expand Down
20 changes: 7 additions & 13 deletions packages/compass-aggregations/src/components/stage-editor/stage-editor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import { StageAutoCompleter } from 'mongodb-ace-autocompleter';

import styles from './stage-editor.module.less';

const INDEX_STATS = '$indexStats';

/**
* Edit a single stage in the aggregation pipeline.
*/
Expand All @@ -17,7 +15,6 @@ class StageEditor extends Component {
static propTypes = {
stage: PropTypes.string,
stageOperator: PropTypes.string,
snippet: PropTypes.string,
error: PropTypes.string,
syntaxError: PropTypes.string,
runStage: PropTypes.func.isRequired,
Expand All @@ -27,7 +24,6 @@ class StageEditor extends Component {
stageChanged: PropTypes.func.isRequired,
isAutoPreviewing: PropTypes.bool.isRequired,
isValid: PropTypes.bool.isRequired,
fromStageOperators: PropTypes.bool.isRequired,
setIsModified: PropTypes.func.isRequired,
projections: PropTypes.array.isRequired,
projectionsChanged: PropTypes.func.isRequired,
Expand Down Expand Up @@ -76,8 +72,6 @@ class StageEditor extends Component {
}

/**
* On update if the stage operator is changed insert the snippet and focus on the editor.
*
* @param {Object} prevProps - The previous properties.
*/
componentDidUpdate(prevProps) {
Expand All @@ -87,9 +81,13 @@ class StageEditor extends Component {
);
this.completer.version = this.props.serverVersion;
if (this.props.stageOperator !== prevProps.stageOperator && this.editor) {
this.editor.setValue('');
this.editor.insertSnippet(this.props.snippet || '');
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's where we used to perform the snippet completion. This would also call the onStageChange twice I believe, and by doing so cause the stage preview to be recalculated. These changes make it so we explicitly try to run now when the operator changes. This might be something we could flag in the store instead.

// Focus the editor when the stage operator has changed.
this.editor.focus();

// When the underlying stage operator changes, re-run the preview.
if (this.props.isAutoPreviewing) {
this.debounceRun();
}
}
}

Expand All @@ -110,11 +108,7 @@ class StageEditor extends Component {
this.props.projectionsChanged();
this.props.setIsModified(true);

if (
(this.props.fromStageOperators === false ||
this.props.stageOperator === INDEX_STATS) &&
this.props.isAutoPreviewing
) {
if (this.props.isAutoPreviewing) {
this.debounceRun();
}
};
Expand Down
1 change: 0 additions & 1 deletion packages/compass-aggregations/src/components/stage-editor/stage-editor.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ describe('StageEditor [Component]', function() {
isValid={isValid}
index={0}
isAutoPreviewing
fromStageOperators={false}
fields={[]}
serverVersion="3.6.0"
runStage={runStageSpy}
Expand Down
6 changes: 0 additions & 6 deletions packages/compass-aggregations/src/components/stage/stage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class Stage extends Component {
sourceName: PropTypes.string,
stage: PropTypes.string.isRequired,
stageOperator: PropTypes.string,
snippet: PropTypes.string,
error: PropTypes.string,
syntaxError: PropTypes.string,
isValid: PropTypes.bool.isRequired,
Expand All @@ -60,7 +59,6 @@ class Stage extends Component {
isComplete: PropTypes.bool.isRequired,
// Can be undefined on the initial render
isMissingAtlasOnlyStageSupport: PropTypes.bool,
fromStageOperators: PropTypes.bool.isRequired,
previewDocuments: PropTypes.array.isRequired,
index: PropTypes.number.isRequired,
isCommenting: PropTypes.bool.isRequired,
Expand Down Expand Up @@ -89,7 +87,6 @@ class Stage extends Component {
shouldComponentUpdate(nextProps) {
const should = (
nextProps.stageOperator !== this.props.stageOperator ||
nextProps.snippet !== this.props.snippet ||
nextProps.error !== this.props.error ||
nextProps.syntaxError !== this.props.syntaxError ||
nextProps.isValid !== this.props.isValid ||
Expand All @@ -98,7 +95,6 @@ class Stage extends Component {
nextProps.isLoading !== this.props.isLoading ||
nextProps.isComplete !== this.props.isComplete ||
nextProps.isMissingAtlasOnlyStageSupport !== this.props.isMissingAtlasOnlyStageSupport ||
nextProps.fromStageOperators !== this.props.fromStageOperators ||
nextProps.index !== this.props.index ||
nextProps.isCommenting !== this.props.isCommenting ||
nextProps.isAutoPreviewing !== this.props.isAutoPreviewing ||
Expand Down Expand Up @@ -150,11 +146,9 @@ class Stage extends Component {
<StageEditor
stage={this.props.stage}
stageOperator={this.props.stageOperator}
snippet={this.props.snippet}
error={this.props.error}
syntaxError={this.props.syntaxError}
isValid={this.props.isValid}
fromStageOperators={this.props.fromStageOperators}
runStage={this.props.runStage}
index={this.props.index}
serverVersion={this.props.serverVersion}
Expand Down
22 changes: 4 additions & 18 deletions packages/compass-aggregations/src/modules/pipeline.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import _reducer, {
STAGE_PREVIEW_UPDATED,
LOADING_STAGE_RESULTS,
STAGE_TOGGLED,
replaceAceTokens,
replaceOperatorSnippetTokens,
INITIAL_STATE,
} from './pipeline';
import { generatePipelineStages } from './pipeline';
Expand Down Expand Up @@ -120,11 +120,7 @@ describe('pipeline module', function () {
expect(
newState[0].stage,
`${name} stage on ${env} env.`
).to.equal(`${comment}${snippet}`);
expect(
newState[0].snippet,
`${name} snippet on ${env} env.`
).to.equal(`${comment}${snippet}`);
).to.equal(replaceOperatorSnippetTokens(`${comment}${snippet}`));
});
});
});
Expand All @@ -134,8 +130,7 @@ describe('pipeline module', function () {
);
const stage = {
stageOperator: geoNear.name,
stage: replaceAceTokens(`${geoNear.comment}${geoNear.snippet}`),
snippet: `${geoNear.comment}${geoNear.snippet}`,
stage: replaceOperatorSnippetTokens(`${geoNear.comment}${geoNear.snippet}`),
};
STAGE_OPERATORS.filter((x) => x.name !== '$geoNear').forEach(
({ name, comment, snippet, env: envs }) => {
Expand All @@ -151,11 +146,7 @@ describe('pipeline module', function () {
expect(
newState[0].stage,
`${name} stage on ${env} env.`
).to.equal(`${comment}${snippet}`);
expect(
newState[0].snippet,
`${name} snippet on ${env} env.`
).to.equal(`${comment}${snippet}`);
).to.equal(replaceOperatorSnippetTokens(`${comment}${snippet}`));
});
}
);
Expand All @@ -165,7 +156,6 @@ describe('pipeline module', function () {
const stage = {
stageOperator: limit.name,
stage: '20',
snippet: `${limit.comment}${limit.snippet}`,
};
STAGE_OPERATORS.filter((x) => x.name !== '$limit').forEach(
({ name, env: envs }) => {
Expand All @@ -182,10 +172,6 @@ describe('pipeline module', function () {
newState[0].stage,
`${name} stage on ${env} env.`
).to.equal('20');
expect(
newState[0].snippet,
`${name} snippet on ${env} env.`
).to.equal('20');
});
}
);
Expand Down
22 changes: 8 additions & 14 deletions packages/compass-aggregations/src/modules/pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ export type Pipeline = {
syntaxError: string | null;
error: string | null;
projections: Projection[];
fromStageOperators?: boolean;
snippet?: string;
isMissingAtlasOnlyStageSupport?: boolean;
executor?: Record<string, unknown>;
}
Expand Down Expand Up @@ -244,7 +242,6 @@ const changeStage = (state: State, action: AnyAction): State => {
const newState = copyState(state);
newState[action.index].stage = action.stage;
newState[action.index].isComplete = false;
newState[action.index].fromStageOperators = false;
return newState;
};

Expand Down Expand Up @@ -326,10 +323,8 @@ const selectStageOperator = (state: State, action: AnyAction): State => {
}
newState[action.index].stageOperator = operatorName;
newState[action.index].stage = value;
newState[action.index].snippet = value;
newState[action.index].isExpanded = true;
newState[action.index].isComplete = false;
newState[action.index].fromStageOperators = true;
newState[action.index].previewDocuments = [];
if (
[SEARCH, SEARCH_META, DOCUMENTS].includes(newState[action.index].stageOperator) &&
Expand All @@ -348,29 +343,28 @@ const selectStageOperator = (state: State, action: AnyAction): State => {
return state;
};

export const replaceOperatorSnippetTokens = (str: string): string => {
const regex = /\${[0-9]+:?([a-z0-9.()]+)?}/ig;
return str.replace(regex, function (_match, replaceWith) {
return replaceWith ?? '';
});
}

const getStageDefaultValue = (stageOperator: string, isCommenting: boolean, env: string): string => {
const operatorDetails = getStageOperator(stageOperator, env);
const snippet = (operatorDetails || {}).snippet || DEFAULT_SNIPPET;
const comment = (operatorDetails || {}).comment || '';
return isCommenting ? `${comment}${snippet}` : snippet;
return replaceOperatorSnippetTokens(isCommenting ? `${comment}${snippet}` : snippet);
};

export const replaceAceTokens = (str: string): string => {
const regex = /\${[0-9]+:?([a-z0-9.()]+)?}/ig;
return str.replace(regex, function (_match, replaceWith) {
return replaceWith ?? '';
});
}

const hasUserChangedStage = (stage: Pipeline, env: string): boolean => {
if (!stage.stageOperator || !stage.stage) {
return false;
}
const value = decomment(stage.stage);
// The default value contains ace specific tokens (${1:name}).
const defaultValue = getStageDefaultValue(stage.stageOperator, false, env);
return value !== replaceAceTokens(defaultValue);
return value !== defaultValue;
};

/**
Expand Down
7 changes: 1 addition & 6 deletions packages/compass-aggregations/src/modules/stage.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ describe('Stage module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$addFields',
stage: `{
totalHomework: { $sum: "$homework" } ,
Expand Down Expand Up @@ -216,7 +215,6 @@ describe('Stage module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$project',
stage: '{_id: 0, avg_price: {$avg: "$price"}}'
};
Expand Down Expand Up @@ -248,7 +246,6 @@ describe('Stage module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$bucket',
stage: `{
groupBy: "$price",
Expand Down Expand Up @@ -302,7 +299,6 @@ describe('Stage module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$count',
stage: '"fieldname"'
};
Expand All @@ -322,7 +318,6 @@ describe('Stage module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$addFields',
stage: `{
isFound:
Expand Down Expand Up @@ -406,7 +401,7 @@ describe('Stage module', function() {

context('when the stage has BSON types', function() {
const stage = {
id: 0, isEnabled: true, isExpanded: true, isValid: true, snippet: '',
id: 0, isEnabled: true, isExpanded: true, isValid: true,
stageOperator: '$match',
stage: '{\n' +
' code: Code(\'some code\'),\n' +
Expand Down
1 change: 0 additions & 1 deletion packages/compass-aggregations/src/modules/update-view.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ describe('large-limit module', function() {
isEnabled: true,
isExpanded: true,
isValid: true,
snippet: '',
stageOperator: '$project',
stage: '{_id: 0, avg_price: {$avg: "$price"}}'
}],
Expand Down
2 changes: 0 additions & 2 deletions packages/compass-saved-aggregations-queries/src/typings.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,8 @@ interface Pipeline {
syntaxError: unknown;
error: unknown;
projections?: unknown[];
fromStageOperators?: boolean;
executor?: unknown;
isMissingAtlasOnlyStageSupport?: boolean;
snippet?: string;
}

type QueryUpdateAttributes = {
Expand Down
3 changes: 0 additions & 3 deletions packages/compass-saved-aggregations-queries/test/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,6 @@ export const pipelines: Item[] = [
syntaxError: null,
error: null,
projections: [],
snippet:
'/**\n * Provide the number of documents to limit.\n */\n${1:number}',
fromStageOperators: false,
executor: {
$limit: 3,
},
Expand Down