Fix packaging #157

sfdctaka · 2024-11-15T22:26:48Z

What does this PR do?

This PR consolidates 3 packages into one. There were 3 packages:

Root. The gateway to extension
LSP client
LSP server

Consolidation allows:

Packaging. npm workspace which was used before is removed because it was using symbolic links to child packages. And npx vsce package wasn't supporting symbolic links thus not producing vsix distributable.
Give us a clean slate to eventually break the single package into multiple packages. Before this PR a clean code sharing was hard and code duplication was made.

sfdctaka · 2024-11-15T22:29:32Z

src/commands/settings/settings.ts


 const SETTING_KEY_SUPPRESS_ALL = 'suppressAll';
 const SETTING_KEY_SUPPRESS_BY_RULE_ID = 'suppressByRuleId';

 export function getUpdateDiagnosticsSettingCommand(
    context: vscode.ExtensionContext
 ): string {
-    return `${getExtensionName(context)}.updateDiagnosticsSetting`;


getExtensionName() was only used here so no need to have a method defined in workspaceUtils.ts.

src/lsp/client/client.ts

src/lsp/server/utils/objectInfoUtils.ts

sfdctaka · 2024-11-15T22:34:43Z

src/lsp/server/utils/serverWorkspace.ts

 import { WorkspaceFolder } from 'vscode-languageserver';

-export class WorkspaceUtils {
-    // WorkspaceFolders is null if Workspace is not configured
+export class ServerWorkspace {


Much of ServerWorkspace came from WorkspaceUtils which most of it was a code from previous iteration prior to LSP. The reason LSP needs a ServerWorkspace was because it wanted to stash workspaceFolders property(next line in the code) that gets passed to when the LSP server is initialized.

Rather than duplicating WorkspaceUtils, I created a property bag class with managing workspaceFolders property. Static methods are essentially the same as they were implemented when LSP feature was done.

Hmm.... I'm not sure how I feel about ServerWorkspace as the name and I think we should stick to the original class+file name. This file contains helper code around the concept of Workspace in an extension, and to that end Server or Client workspace doesn't make sense (not to me at least).... but I'll differ to others on this one.

Discussed offline and will keep as is in this PR

WorkSpaceFolder in the LSP server and VS Code are two distinct classes. In the previous workspace structure, the server and client were independent packages, so using workspace as a name made sense within a self-contained project. Now that we've consolidated everything into a single package, renaming it to ServerWorkSpace is more appropriate to distinguish it from VS Code's workspace.

sfdctaka · 2024-11-15T22:36:00Z

src/utils/constants.ts

+export const CORE_EXTENSION_ID = 'salesforce.salesforcedx-vscode-core';
+export const SFDX_PROJECT_FILE = 'sfdx-project.json';


These constants, too, were originally defined in root, which moved to LSP server and then reverse exported back to the root project. Since we're consolidating they're back to where they were.

sfdctaka · 2024-11-15T22:36:37Z

test/TestHelper.ts

+    key: 'target-org'
+};
+
+export class TempProjectDirManager {


TempProjectDirManager coming back to the file where it was originally.

test/suite/lsp/server/diagnostics/gql/over-sized-record.test.ts

sfdctaka · 2024-11-15T22:44:00Z

src/lsp/client/client.ts

@@ -24,7 +24,7 @@ export function activate(
 ) {


This file is renamed from extension.ts to client.ts. Extension.ts should be used for the main entry point in an extension. So following the same spirit as lsp/server I named it client.ts. I thought of changing activate() too since it could be anything if it is not an extension. Besides, it's taking extra parameters in updateDiagnosticSettingCommand and diagnosticSettingSection that aren't expected by extension loader. But changing the file name suffices so I left activate() as is.

src/lsp/server/utils/serverWorkspace.ts

maliroteh-sf

Left some comments for your considerations. For some of the feedback we can see how the rest of the team feels about them so don't feel obligated to change things right away... if others are ok with the way things are in your PR then we can go as is.

tsconfig.json

.vscodeignore

package.json

khawkins · 2024-11-18T23:09:29Z

Nit: Do we need a yaml/ subdirectory under resources/? Seems like a folder structure that would benefit from being as flat as possible, at least for this project.

sfdctaka · 2024-11-18T23:12:51Z

@khawkins yaml/ folder is not necessary. I have it so that it would look like it's logically divided like its sibling folders instructions and templates. I'll remove it.

khawkins · 2024-11-18T23:34:00Z

We lost esbuild as part of #127. We can't package without it.

ben-zhang-at-salesforce

looks good for me, great job.

Co-authored-by: Kevin Hawkins <khawkins@salesforce.com>

khawkins · 2024-11-19T18:41:38Z

package.json

+        "compile": "rimraf ./out/tsconfig.tsbuildinfo && tsc -b ./ && npm run copy-yaml",
+        "copy-yaml-win": "mkdir out\\src\\resources && copy resources\\component-experiences.yaml out\\src\\resources",
+        "copy-yaml-unix": "mkdir -p ./out/src/resources && cp ./resources/component-experiences.yaml ./out/src/resources",
+        "copy-yaml": "npm run copy-yaml-win || npm run copy-yaml-unix",


This...is not my favorite, relying on fatal errors in the shell to take affirmative alternative action. It can be risky, and is otherwise not super intuitive. A more standard and maintainable way to do this would be to create a basic Node utility module to manage the logic. We know we have a Node.js environment in either case, so we can write a pretty simple OS-agnostic script with fs, path, and os to do the copying, give it an identifiable name, and voila, maintainable code. 🙂

It's slightly more code than this, but IMO it's easier to maintain, and more intuitive to understand.

Let me take care in another PR. (This PR got merged few minutes ago.)

I think I remember why I had yaml folder under resources folder. It's because I was able to use cp -r. Since the yaml file was move and cp was failing on copying a single file claiming it can't find resources folder.

haifeng-li-at-salesforce

LGTM. Great job!

khawkins · 2024-11-19T18:46:15Z

src/lsp/client/client.ts

@@ -60,8 +60,8 @@ export function activate(

    // Create the language client and start the client.
    client = new LanguageClient(
-        'LSP Mobile',
-        'LSP Mobile Client',
+        'Salesforce Mobile Language Server',


This first item would typically be an identifier, e.g. salesforceMobileLanguageServer.

👍
Will take care of this along with the above feedback that you made.

sfdctaka commented Nov 15, 2024

View reviewed changes

src/lsp/client/client.ts Outdated Show resolved Hide resolved

sfdctaka commented Nov 15, 2024

View reviewed changes

src/lsp/server/utils/objectInfoUtils.ts Outdated Show resolved Hide resolved

sfdctaka commented Nov 15, 2024

View reviewed changes

test/suite/lsp/server/diagnostics/gql/over-sized-record.test.ts Outdated Show resolved Hide resolved

sfdctaka commented Nov 15, 2024

View reviewed changes

maliroteh-sf reviewed Nov 16, 2024

View reviewed changes

src/lsp/server/utils/serverWorkspace.ts Outdated Show resolved Hide resolved

maliroteh-sf reviewed Nov 16, 2024

View reviewed changes

khawkins reviewed Nov 18, 2024

View reviewed changes

tsconfig.json Show resolved Hide resolved

khawkins reviewed Nov 18, 2024

View reviewed changes

.vscodeignore Show resolved Hide resolved

khawkins reviewed Nov 18, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

sfdctaka force-pushed the fixPackaging branch 14 times, most recently from 6861c18 to ddac535 Compare November 19, 2024 07:41

sfdctaka force-pushed the fixPackaging branch 5 times, most recently from 865a22b to be50e99 Compare November 19, 2024 08:22

ben-zhang-at-salesforce approved these changes Nov 19, 2024

View reviewed changes

sfdctaka and others added 17 commits November 19, 2024 09:21

Fix path to server module.

07df80a

Consume lsp module directly.

41446f2

touch up after rebase.

69b2f35

minify.

4edc978

caps.

c31178f

single package to rule them all.

c92bd36

prettier.

eb27dc0

fix test.

d49d089

minify.

feda547

Rename. Tests. Touch ups.

ab70caf

Packages versions.

9544330

Cleanup.

1fd7472

Update package.json

8244685

Co-authored-by: Kevin Hawkins <khawkins@salesforce.com>

Feedback from code review.

4c5370c

Fix windows.

04cb99f

Rename.

852d395

Change working directory for test.

0a90efa

sfdctaka force-pushed the fixPackaging branch from be50e99 to 0a90efa Compare November 19, 2024 17:21

Codecov update.

119864b

khawkins reviewed Nov 19, 2024

View reviewed changes

haifeng-li-at-salesforce approved these changes Nov 19, 2024

View reviewed changes

sfdctaka merged commit 23757e6 into salesforce:main Nov 19, 2024
7 checks passed

sfdctaka deleted the fixPackaging branch November 19, 2024 18:44

khawkins reviewed Nov 19, 2024

View reviewed changes

		export const CORE_EXTENSION_ID = 'salesforce.salesforcedx-vscode-core';
		export const SFDX_PROJECT_FILE = 'sfdx-project.json';

Fix packaging #157

Fix packaging #157

Uh oh!

Conversation

sfdctaka commented Nov 15, 2024

What does this PR do?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maliroteh-sf Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maliroteh-sf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khawkins commented Nov 18, 2024

Uh oh!

sfdctaka commented Nov 18, 2024

Uh oh!

khawkins commented Nov 18, 2024

Uh oh!

ben-zhang-at-salesforce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haifeng-li-at-salesforce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maliroteh-sf Nov 15, 2024 •

edited

Loading