Skip to content

Fix missing __dirname in --locate-shell-integration-path#231423

Merged
Tyriar merged 1 commit intomicrosoft:mainfrom
jamesharris-garmin:fix-server-cli
Oct 15, 2024
Merged

Fix missing __dirname in --locate-shell-integration-path#231423
Tyriar merged 1 commit intomicrosoft:mainfrom
jamesharris-garmin:fix-server-cli

Conversation

@jamesharris-garmin
Copy link
Copy Markdown
Contributor

@jamesharris-garmin jamesharris-garmin commented Oct 15, 2024

This patch fixes #230584 by applying the recommended fix from:

https://masteringjs.io/tutorials/node/__dirname-is-not-defined

@jamesharris-garmin jamesharris-garmin mentioned this pull request Oct 15, 2024
Closed
@Tyriar Tyriar requested a review from bpasero October 15, 2024 17:53
@Tyriar Tyriar added this to the October 2024 milestone Oct 15, 2024
Tyriar
Tyriar reviewed Oct 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

lgtm @bpasero could you verify this is the right way to do this now?

@Tyriar Tyriar enabled auto-merge October 15, 2024 17:53
bpasero
bpasero approved these changes Oct 15, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Yes, it is:

vscode/src/main.ts

Line 24 in 6200f19

const __dirname = path.dirname(fileURLToPath(import.meta.url));

@bpasero
Copy link
Copy Markdown
Member

bpasero commented Oct 15, 2024

But I would not call out "ESM" specifically, this is just the way of doing it now 🤔

@Tyriar Tyriar merged commit 58cab99 into microsoft:main Oct 15, 2024
@bpasero
Copy link
Copy Markdown
Member

bpasero commented Oct 15, 2024 •
edited
Loading

Follow up #231427

This is really a good catch and was missed during the ESM migration. We are bitten by the fact that the node.js types declare a global __dirname :-/

Update: I think we can have a ESLint rule to disallow...

@jamesharris-garmin jamesharris-garmin deleted the fix-server-cli branch October 15, 2024 19:53
@jamesharris-garmin
Copy link
Copy Markdown
Contributor Author

jamesharris-garmin commented Oct 15, 2024

Thanks everyone for the quick turn around on this.

@prasanthcakewalk
Copy link
Copy Markdown

prasanthcakewalk commented Oct 16, 2024

May not be worth changing anything, but I think the import here:

vscode/src/vs/server/node/server.cli.ts

Line 10 in ffa1857

import path from 'path';

is not necessary, since dirname has already been imported here:

vscode/src/vs/server/node/server.cli.ts

Line 12 in ffa1857

import { dirname, extname, resolve, join } from '../../base/common/path.js';

I think the import line could be removed and
const __dirname = path.dirname(url.fileURLToPath(import.meta.url));
could just be changed to
const __dirname = dirname(url.fileURLToPath(import.meta.url));
I could be wrong though; I'm not a typescript developer.

@bpasero
Copy link
Copy Markdown
Member

bpasero commented Oct 16, 2024

Yes thanks, will address that.

NikolaRHristov pushed a commit to CodeEditorLand/Editor that referenced this pull request Oct 16, 2024
@Tyriar @NikolaRHristov
Merge pull request microsoft#231423 from jamesharris-garmin/fix-serve…
b8d7abd 
…r-cli

Fix missing __dirname in --locate-shell-integration-path
@bpasero bpasero mentioned this pull request Oct 28, 2024
Merged
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bpasero bpasero bpasero approved these changes

+1 more reviewer

@Tyriar Tyriar Tyriar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

October 2024

Development

Successfully merging this pull request may close these issues.

WSL terminal CLI is broken

4 participants

@jamesharris-garmin @bpasero @prasanthcakewalk @Tyriar