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

JavaScript and Node.js walkthrough #157965

Merged
merged 20 commits into from
Sep 14, 2022

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Aug 12, 2022

I figured it would be worth opening a new PR for this one.

The feedback I got on #151665 was effectively to scope things down and make a walkthrough that's just Node.js and JavaScript. Doing that brings some parity with other languages like Python and reduces the need to introduce HTML, at the expense of losing the convenience of the browser and web development.

Fixes microsoft/TypeScript#48489
Ticks a box off on #144062

Here's the current TODOs, with some specific callouts that I'd appreciate some input on.

  • Media Assets: I'll need to work with @misolori @daviddossett on these.
    • Walkthrough icon
    • Media for each step (currently just a TODO page)
  • Workflow and Phrasing
    • I added a button to restart VS Code in case node isn't present on the terminal. Should I?
    • Is there a better way to have a user graduate from running on the terminal to debugging?
  • Polish
    • Can we either hide or fill in the step for installing Node.js? Is there an easy way to determine whether Node.js is already installed? (@mjbvz)
    • In the last step, if a user tries to run/debug a single JavaScript file in their root directory, the debugger takes forever to start. Can we fix this? (@connor4312)
  • Experiments: Let's plumb these through (@devinvalenciano @isidorn @digitarald). Good experiments to start with:
    • How do we pitch this? Is it a "get started with JS" or a "get started with Node.js and JS" walkthrough?

Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Looks like a great start!

For determining if node is on the users's system, I think you'll have to use child_process to check. Hopefully we can catch the majority of installs. Here's an example of how we find git:

function findGitDarwin(onValidate: (path: string) => boolean): Promise<IGit> {

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 12, 2022

@mjbvz seems fine but then I have to do that every time the extension is activated, right? It seems wasteful to spawn that process the first time a .ts/.js file is opened. I can do it, but it probably makes sense for this to live in its own extension long-term.

@DanielRosenwasser
Copy link
Member Author

Actually, I seem to be having some trouble getting the walkthrough wired up with that. I'll push up my new changes in a minute but the current state is this:

I've defined a new command

// extensions/typescript-language-features/src/commands/jsWalkthrough.ts
// ...

export class NodeInstallationFoundCommand {
	public static readonly id = 'javascript-walkthrough.commands.nodeInstallationFound';
	public readonly id = 'javascript-walkthrough.commands.nodeInstallationFound';
	public execute() {
		vscode.window.showInformationMessage('yay it happened');
	}
}

and registered it

// extensions/typescript-language-features/src/commands/index.ts
// ...

	commandManager.register(new NodeInstallationFoundCommand());

and fired the command after it's registered and after I check for if Node exists.

// extensions/typescript-language-features/src/extension.ts
// ...

	hasNode().then(
		async (isInstalled) => {
			if (isInstalled) {
				await vscode.commands.executeCommand('javascript-walkthrough.commands.nodeInstallationFound');
				vscode.window.showInformationMessage('it is installed :)');
			}
			else {
				vscode.window.showErrorMessage('it aint installed :(');
			}
		}
	);

When the extension gets activated, I do see the information message; however, in the walkthrough I have an onCommand completion event

          {
            "id": "walkthroughs.jsWelcome.downloadNode.forMacOrWindows",
            "title": "%walkthroughs.jsWelcome.downloadNode.forMacOrWindows.title%",
            "description": "%walkthroughs.jsWelcome.downloadNode.forMacOrWindows.description%",
            "media": {
              "markdown": "resources/walkthroughs/TODO.md"
            },
            "when": "isWindows || isMac",
            "completionEvents": [
              "onLink:https://nodejs.org/en/download/",
              "onCommand:javascript-walkthrough.commands.nodeInstallationFound"
            ]
          },

but it doesn't seem like that step is marked as completed.

Anything obvious I'm doing wrong?

@mjbvz mjbvz added this to the September 2022 milestone Aug 17, 2022
@DanielRosenwasser
Copy link
Member Author

Hey all, here's the current status.

First, the walkthrough just has 4 steps now

  • Install Node.js
  • Create a JS File
  • Run & Debug the JS File
  • Read more

The notable stuff on each step is

  • "Install Node.js" still segments you based on Linux vs Windows/Mac. I don't bother to do any auto-detection to see whether Node.js is already installed. This is partially because the API seems to have issues with marking a step as completed programmatically. @karrtikr got around this in Improve how to notify the user they don't have Python installed when installing the Python extension for the first time vscode-python#19379 by just hiding the tile with a context variable. I'm probably not going that route in this PR.
  • "Create a JS File" is minimal. I just use console.log.
  • "Run & Debug" is merged into a single step.
    • I mention running from the terminal, but I don't present a button to do it.

    • I do check that Node was installed when clicking the debug button. I give the user the option of Reload VS Code, Try Debugging Anyway, and Dismiss.

      Run and Debug Failure Dialog

I will need assets for each step, along with a walkthrough icon which should probably use the JS community logo, the Node.js logo, or both. I've created some tentative assets, which I derived from the Python walkthrough, but they don't appear aligned, so I need some help on that.

I don't expect this to go into the August release at this point, but the sooner the better.

@daviddossett
Copy link
Contributor

I'll add the assets to this PR this week 👍

@daviddossett
Copy link
Contributor

Updated icon and SVGs

CleanShot.2022-09-02.at.09.55.04.mp4

Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

The icon.png file is used in VS Code's extension UI. I think it would be confusing to use a js specific one for the JS/TS language features extension

@daviddossett
Copy link
Contributor

The icon.png file is used in VS Code's extension UI. I think it would be confusing to use a js specific one for the JS/TS language features extension

That's fair. I'll replace with something less specific.

@DanielRosenwasser
Copy link
Member Author

Huh - is there no way to provide an alternative icon for a walkthrough? (@lramos15)

@lramos15
Copy link
Member

Not sure I understand the question, what do you mean by alternative icon @DanielRosenwasser

@daviddossett
Copy link
Contributor

Not sure I understand the question, what do you mean by alternative icon @DanielRosenwasser

If I understood correctly I think he means that these icons are currently both using the same source icon. I.e. you can't use a different icon for a walkthrough vs. the extension itself.

CleanShot 2022-09-13 at 10 14 13@2x

CleanShot 2022-09-13 at 10 14 24@2x

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Sep 13, 2022

That's correct - but I don't think we should block this PR over the icon. I would rather begin experimenting here, and in the future use a different icon if walkthroughs eventually do support their own icons.

mjbvz
mjbvz previously approved these changes Sep 13, 2022
@mjbvz mjbvz merged commit 0cbcb1b into microsoft:main Sep 14, 2022
bpasero added a commit that referenced this pull request Sep 15, 2022
bpasero added a commit that referenced this pull request Sep 15, 2022
Revert "JavaScript and Node.js walkthrough (#157965)"

This reverts commit 0cbcb1b.
@DanielRosenwasser DanielRosenwasser deleted the nodejsWalkthrough branch October 12, 2022 23:31
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contribute to the welcome view/walkthrough
4 participants