Skip to content

W-18851407 fix: delete forked code dependency and cleanup #611

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

madhur310
Copy link

What does this PR do?

Deleting forked code and using dependency instead, refactoring and moving code out of LS-common package where its not needed.

What issues does this PR fix or reference?

@W-18851407@

@madhur310 madhur310 requested a review from a team as a code owner August 6, 2025 00:27
@madhur310 madhur310 requested review from diyer and mshanemc and removed request for diyer August 6, 2025 00:27
Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

always review the AI

@@ -1,12 +1,12 @@
{
"compilerOptions": {
"target": "es2017",
"target": "es2018",
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -454,7 +463,7 @@ export default class Server {
const match = findDynamicContent(content, relativeOffset);

if (match) {
const item = iterators.find(i => i.name === match) || null;
const item = iterators.find((i) => i.name === match) || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is pretty far behind, eslint-wise, but prefer ?? over ||.

It doesn't matter until it does (falsy 0, etc) so it's a good rule/habit.

package.json Outdated
@@ -78,5 +79,11 @@
"commitizen": {
"path": "./node_modules/cz-conventional-changelog"
}
},
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see that any of these were uesd. AI slop?

Copy link
Author

Choose a reason for hiding this comment

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

doh!

@@ -31,6 +31,7 @@
"acorn": "^6.0.0",
"acorn-loose": "^6.0.0",
"acorn-walk": "^6.0.0",
"bs-logger": "^0.2.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

also check this one?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the catch.

"vscode-nls": "^4.1.2",
"vscode-uri": "1.0.6"
},
"devDependencies": {
"@jest/reporters": "^29.7.0",
"@types/babel-types": "^7.0.8",
"@types/fs-extra": "^11.0.4",
"@types/glob": "^7.1.3",
"@types/glob": "^8.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

glob isn't used...why do you need types?

Copy link
Author

Choose a reason for hiding this comment

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

Glob is being used:
packages/lightning-lsp-common/src/utils.ts
packages/lightning-lsp-common/src/context.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

in meant in this package/package.json.

weird to see types in devDependencies but the actual glob package nowhere in aura-language-server?

collector(info.name, info, info.type);
}
}
function getAttributesData(tag: string): any[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

are they really any ? Seems to be expecting a very specific set of props.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the catch!

}
function getTagsData(): { name: string; description?: string; attributes: any[] }[] {
const tags: { name: string; description?: string; attributes: any[] }[] = [];
for (const [tag, tagInfo] of getAuraTags()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could just return an array from mapping getAuraTags instead of mutating an array

@@ -31,6 +31,7 @@
"acorn": "^6.0.0",
"acorn-loose": "^6.0.0",
"acorn-walk": "^6.0.0",
"bs-logger": "^0.2.6",
"change-case": "^3.1.0",
"enhanced-resolve": "^2.2.2",
"fs-extra": "^11.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get rid of fs-extra and use native?

Copy link
Author

Choose a reason for hiding this comment

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

it seems very extensively used, i will create a follow up to do that.

if (
// LWC component
(/.*(.ts|.js)$/.test(ext) && folder === name && parentFolder === 'lwc') ||
(/.*(.ts|.js)$/.test(ext) && folder === name && parentFolder === 'lwc') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

endsWith?

@@ -30,7 +30,7 @@ export class LWCDataProvider implements IHTMLDataProvider {
}

provideTags(): ITagData[] {
const customTags = this.indexer.customData.map(tag => {
const customTags = this.indexer.customData.map((tag) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi, you can omit the extra curlies+return by

.map((tag) => ({name:, etc})

Copy link
Author

Choose a reason for hiding this comment

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

yep, I just auto fixed the issues - will update it.

}

describe('basic functionality', () => {
it('should extract property from v binding in attribute', () => {
Copy link
Author

Choose a reason for hiding this comment

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

cursor was really bad at writing these unit tests, so had to do them manually (hence the helper function :)).

@@ -1,5 +1,11 @@
// NOTE: This script is not used in the build process, but needs to be run manually to update the aura-system.json file
Copy link
Contributor

Choose a reason for hiding this comment

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

when does that happen? We should have a doc explaining when you need to do steps like that.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if anyone has done this step in years, the aura-system.json file seems quite old. I didn't feel like deleting the script was right thing. I will bring it up in stand up to see if others have ideas too.

About doing this step - I guess we could do it, but source of this file seems unknown so we would end up generating the same content.

Copy link
Contributor

Choose a reason for hiding this comment

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

does that move/rename somehow keep it out of the compiled code? If not, maybe a tsconfig change could?

Copy link
Author

Choose a reason for hiding this comment

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

yeah it does, I see only transformed-lwc-standard.json in the lib folder.

@@ -189,7 +189,7 @@ describe('handlers', () => {

await server.onInitialize(initializeParams);
const completions = await server.onCompletion(params);
const labels = completions.items.map(item => item.label);
const labels = completions.items.map((item) => item.label);
Copy link
Contributor

Choose a reason for hiding this comment

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

creating a getLabels(initializeParams) =>
might save you a lot of clutter

expect(value).toBe('c-my-card');
});

it('treats interop as lightning', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what was bad about these tests?

Copy link
Author

@madhur310 madhur310 Aug 10, 2025

Choose a reason for hiding this comment

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

The code they were testing was dead code - I removed the code for tagFromFile, tagFromDirectory

"mock-fs": "^5.5.0",
"prettier": "^2.0.5",
"shelljs": "^0.8.5",
"tmp": "^0.0.33",
"ts-jest": "^29.2.6",
"typescript": "5.0.4"
"typescript": "5.0.4",
"xregexp": "^4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's that?

maybe go through all the deps and make sure they're used/needed?

@madhur310 madhur310 requested a review from mshanemc August 10, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants