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

Finalize draft not aware diagnostics for getRelatedListRecords and getRelatedListCount #147

18 changes: 12 additions & 6 deletions lsp/server/src/diagnostic/js/adapters-local-change-not-aware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import { DiagnosticProducer } from '../DiagnosticProducer';
import { Diagnostic, DiagnosticSeverity } from 'vscode-languageserver/node';

export const LOCAL_CHANGE_NOT_AWARE_MESSAGE =
'The wire adapter you are using allows you to work offline, but it does not automatically update its records when data is added or removed while you are disconnected.';
'You are using a wire adapter that will work with records while offline, but will not update to add or remove records that are created or deleted while offline. Consider using GraphQL to create a related list with records that are editable offline.';
export const LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL =
'https://developer.salesforce.com/docs/platform/graphql/guide/query-related-list-info.html';
const SEVERITY = DiagnosticSeverity.Information;

const LOCAL_CHANGE_NOT_AWARE_ADAPTERS: string[] = [
Expand Down Expand Up @@ -45,22 +47,26 @@ export class AdaptersLocalChangeNotAware implements DiagnosticProducer<Node> {
start: textDocument.positionAt(item.start as number),
end: textDocument.positionAt(item.end as number)
},
message: LOCAL_CHANGE_NOT_AWARE_MESSAGE
message: LOCAL_CHANGE_NOT_AWARE_MESSAGE,
code: LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

diagnostic renders code and codeDescription as link

codeDescription: {
href: LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL
}
} as Diagnostic;
})
);
}

/**
* Find @wire adapter call which called in the local change not aware adapters. For example:
* Find a list of adapters which are not draft change aware. For example: getRelatedListRecords from below LWC.
export default class RelatedListRecords extends LightningElement {
...
@wire(getRelatedListRecords,
...
}
* @param astNode root node to search
* @param adapterNames adapter which are not able to reflect the local change.
* @returns nodes with adapter name
* @param astNode The AST root node for the LWC js file.
* @param adapterNames Adapter names which are not able to reflect the local change.
* @returns A list of AST nodes whose names match the adapter names.
*/
private findLocalChangeNotAwareAdapterNode(
astNode: Node,
Expand Down
9 changes: 5 additions & 4 deletions lsp/server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,16 @@ documents.onDidClose((e) => {
});

connection.languages.diagnostics.on(async (params) => {
const document = documents.get(params.textDocument.uri);
if (document !== undefined) {
const uri = params.textDocument.uri;
const document = documents.get(uri);

// Do diagnostics if document is under LWC folder and already in cache.
if (uri.indexOf(WorkspaceUtils.LWC_PATH) > 0 && document !== undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only do diagnostics for file under lwc folder

return {
kind: DocumentDiagnosticReportKind.Full,
items: await validateDocument(settings, document, extensionTitle)
} satisfies DocumentDiagnosticReport;
} else {
// We don't know the document. We can either try to read it from disk
// or we don't report problems for it.
return {
kind: DocumentDiagnosticReportKind.Full,
items: []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,13 @@ import * as sinon from 'sinon';

import { suite, test, afterEach } from 'mocha';

import { AdaptersLocalChangeNotAware } from '../../../diagnostic/js/adapters-local-change-not-aware';
import {
AdaptersLocalChangeNotAware,
LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL
} from '../../../diagnostic/js/adapters-local-change-not-aware';
import { parseJs } from '../../../utils/babelUtil';
import { TextDocument } from 'vscode-languageserver-textdocument';

const relatedRecordsJS = `
import { LightningElement, wire } from "lwc";
import { getRelatedListRecords } from "lightning/uiRelatedListApi";

export default class RelatedListRecords extends LightningElement {

recordId = "0015g00000XYZABC";

relatedRecords;

@wire(getRelatedListRecords, {
parentRecordId: "$recordId",
relatedListId: "Opportunities",
fields: ["Opportunity.Name"],
})
relatedListHandler({ error, data }) {
}
}
`;

suite(
'JS Diagnostics Test Suite - Server - Adapter Local Change Not Aware',
() => {
Expand All @@ -44,30 +27,79 @@ suite(
});

test('Wire "getRelatedRecords" produces local-change-not-aware diagnostic', async () => {
const js = `
import { LightningElement, wire } from "lwc";
import { getRelatedListRecords } from "lightning/uiRelatedListApi";

export default class RelatedListRecords extends LightningElement {
@wire(getRelatedListRecords, {})
onResultHandler({ error, data }) {
};
}
`;
const textDocument = TextDocument.create(
'file://test.js',
'javascript',
1,
relatedRecordsJS
js
);
const jsAstNode = parseJs(textDocument.getText());
const jsAstNode = parseJs(js);
const diagnostics = await rule.validateDocument(
textDocument,
jsAstNode
);

assert.equal(diagnostics.length, 1);
const { range } = diagnostics[0];
const { range, codeDescription } = diagnostics[0];

const startOffset = textDocument.offsetAt(range.start);
const endOffset = textDocument.offsetAt(range.end);

const targetString = relatedRecordsJS.substring(
startOffset,
endOffset
);
const targetString = js.substring(startOffset, endOffset);

assert.equal(targetString, 'getRelatedListRecords');
assert.equal(
codeDescription?.href,
LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL
);
});

test('Wire "getRelatedListCount" produces local-change-not-aware diagnostic', async () => {
const js = `
import { LightningElement, wire } from "lwc";
import { getRelatedListCount } from "lightning/uiRelatedListApi";

export default class RelatedListRecords extends LightningElement {
@wire(getRelatedListCount, {})
onResultHandler({ error, data }) {
};
}
`;
const textDocument = TextDocument.create(
'file://test.js',
'javascript',
1,
js
);
const jsAstNode = parseJs(js);
const diagnostics = await rule.validateDocument(
textDocument,
jsAstNode
);

assert.equal(diagnostics.length, 1);
const { range, codeDescription } = diagnostics[0];

const startOffset = textDocument.offsetAt(range.start);
const endOffset = textDocument.offsetAt(range.end);

const targetString = js.substring(startOffset, endOffset);

assert.equal(targetString, 'getRelatedListCount');
assert.equal(
codeDescription?.href,
LOCAL_CHANGE_NOT_AWARE_EXTERNAL_DOC_URL
);
});
}
);
5 changes: 0 additions & 5 deletions lsp/server/src/validateDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,5 @@ export async function validateDocument(
results = results.concat(diagnostics);
}

// Set the source for diagnostic source.
results.forEach((diagnostic) => {
diagnostic.source = extensionName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why did you get rid of this? I wasn't a fan of this post processing anyway so removing it is a welcome change but where is the source set now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vscode has its own brain where and how to render the diagnostic attributes. I like to render the diagnostic.source "Salesforce Mobile Extension in ui so to tell developer where this diagnostic is from. When we try to render a link in diagnostic pop up like what Kevin wants. the source is rendered between the msg and cde/codeDescription like below. Totally messed up the effect we want. Removing the source makes the popup close to what we want.
image

Source will not be set if we will render the link.
Kevin is thinking of what will be finally word for the rules, if there's no link needed. we can put the source back.

Copy link
Contributor

Choose a reason for hiding this comment

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

To make myself clear, I wasn't the fan of post processing, so removing is fine I think. But do what you think is the best thing to do.

});

return results;
}
5 changes: 4 additions & 1 deletion src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ export function activate(context: vscode.ExtensionContext) {

const command = getUpdateDiagnosticsSettingCommand(context);

lspClient.activate(context, command, SECTION_DIAGNOSTICS);
// Only initialize the LSP if opened project is a SFDX project.
if (WorkspaceUtils.isSfdxProjectOpened()) {
lspClient.activate(context, command, SECTION_DIAGNOSTICS);
}
}

// This method is called when your extension is deactivated
Expand Down
Loading