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

feat: configure sfdx workspace to write tsconfig files #591

Merged
merged 10 commits into from
Jul 8, 2024

Conversation

jmsjtu
Copy link
Collaborator

@jmsjtu jmsjtu commented Jun 26, 2024

What does this PR do?

This PR adds the ability for the language server to generate a tsconfig.json in addition to the jsconfig.json.

The language server will generate a tsconfig.sfdx.json file in the .sfdx directory and a tsconfig.json in each lwc directory.

The feature is gated behind a feature flag that will be implemented in the salesforcedx-vscode repository.

What issues does this PR fix or reference?

@W-15644075@

@jmsjtu jmsjtu requested a review from a team as a code owner June 26, 2024 01:09
@jmsjtu jmsjtu requested a review from peternhale June 26, 2024 01:09
@@ -130,6 +131,15 @@ export default class Server {
};
}

async onInitialized(): Promise<void> {
// The config value comes from salesforcedx-vscode/salesforcedx-vscode-lwc
const hasTsEnabled = await this.connection.workspace.getConfiguration('salesforcedx-vscode-lwc.preview.typeScriptSupport');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The entire feature is gated behind this feature flag, which hasn't been implemented in salesforcedx-vscode yet.

There is a follow-up PR to implement the flag, in the meantime, I've tested locally that when the config value is enabled it works correctly.

Copy link
Collaborator Author

@jmsjtu jmsjtu Jun 26, 2024

Choose a reason for hiding this comment

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

After discussing with the team, we should remove the tsconfig.json if salesforcedx-vscode-lwc.preview.typeScriptSupport = false.

@@ -80,6 +80,7 @@ export default class Server {

constructor() {
this.connection.onInitialize(this.onInitialize.bind(this));
this.connection.onInitialized(this.onInitialized.bind(this));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like config values are not available during onInitialize which is why I'm using onInitialized instead.

Once the feature is GA, we can move everything back to onInitialize.

const tsConfigTemplate = await fs.readFile(utils.getSfdxResource('tsconfig-sfdx.json'), 'utf8');
const forceignore = path.join(this.workspaceRoots[0], '.forceignore');
// TODO: We should only be looking through modules that have TS files
const modulesDirs = await this.getModulesDirs();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a follow-up PR I plan to only check for lwc directories that actually have TS files.

For now, I'm writing to all lwc directories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should also only be generating either a jsconfig.json or a tsconfig.json but not both, this can be done once we've moved to GA.

jmsjtu and others added 2 commits June 26, 2024 10:54
….base.json

Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
@mingxuanzhangsfdx mingxuanzhangsfdx requested review from mingxuanzhangsfdx and removed request for a team and peternhale June 26, 2024 22:13
@mingxuanzhangsfdx
Copy link
Member

mingxuanzhangsfdx commented Jun 26, 2024

NIT: In order to pass the step validate pr in CI you have to follow the format in the description like I just updated

What issues does this PR fix or reference?
@[W-The work item ID]@

Copy link
Member

@mingxuanzhangsfdx mingxuanzhangsfdx left a comment

Choose a reason for hiding this comment

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

Looks good!

const relativeWorkspaceRoot = utils.relativePath(path.dirname(tsConfigPath), this.workspaceRoots[0]);
const tsConfigContent = this.processTemplate(tsConfigTemplate, { project_root: relativeWorkspaceRoot });
this.updateConfigFile(tsConfigPath, tsConfigContent);
await this.updateForceIgnoreFile(forceignore);
Copy link
Member

Choose a reason for hiding this comment

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

What is the point here to call this.updateForceIgnoreFile? The file you are adding is tsconfig.json here. but the function is to add jsconfig and eslintrc into ignore file. Are you trying to ignore the tsconfig.json files?

Copy link
Member

Choose a reason for hiding this comment

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

nvm I saw the PR you created

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the same behavior as with jsconfig.json, if the language server detects a tsconfig.json it will add it to the ignore file.

@jmsjtu jmsjtu merged commit 54e6dc0 into forcedotcom:main Jul 8, 2024
10 checks passed
@jmsjtu jmsjtu deleted the jtu/tsconfig branch July 8, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants