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

Plan for normalizing samples #110

Closed
18 of 24 tasks
octref opened this issue Oct 17, 2018 · 35 comments
Closed
18 of 24 tasks

Plan for normalizing samples #110

octref opened this issue Oct 17, 2018 · 35 comments
Assignees
Milestone

Comments

@octref
Copy link
Contributor

octref commented Oct 17, 2018

In the new API doc, sample is tightly integrated into the API docs:

The samples become an important resource for people to explore VS Code API. We should try to make the samples

  • More consistent.
  • Closer to the HelloCode sample, which we suppose each Extension API reader will become familiar with.

This task is for you to update your sample to meet the Sample Guideline. It shouldn't take more than 20 minutes for you.


Please work on the ext-docs branch.
ext-docs branch has been merged to master. Work on master branch.

I did a PR to update the webview sample: #111. The sample and the Webview Guide tie nicely together now.


Deferred: Please finish this during debt week.

@octref octref added this to the October 2018 milestone Oct 17, 2018
octref added a commit that referenced this issue Oct 17, 2018
Update webview sample for Sample Guideline for #110
@chrmarti chrmarti self-assigned this Oct 18, 2018
@jrieken
Copy link
Member

jrieken commented Oct 22, 2018

@octref Why do we use prettier and not our formatters? Apart from an extra config file, this looks like we are telling the world "Don't use our stuff"

@octref octref reopened this Oct 22, 2018
@octref
Copy link
Contributor Author

octref commented Oct 22, 2018

@jrieken

  1. Your user config might have formatting configs, but I hope the formatter formats everything according to the workspace configuration.
  2. Neither our formatter do not relate to https://github.com/vvakame/typescript-formatter, therefore users end up in a situation when they can't consistently format files in both editor + CLI.
  3. tsfmt offers too much options and might turn new users away. It reads tsconfig, tslint config, editorconfig, vscodeconfig, and tsfmt.json. I don't want my formatting operation to be based on so many configs in a non-transparent way.

@jrieken
Copy link
Member

jrieken commented Oct 22, 2018

Your user config might have formatting configs, but I hope the formatter formats everything according to the workspace configuration.

No, I use the default formatter options, e.g for TypeScript. Now prettifier makes TypeScript code look different then TypeScript makes it look and that's wrong if you ask me.

users end up in a situation when they can't consistently format files in both editor + CLI

These are samples right? Why would anyone want to format them from the CLI?

tsfmt offers too much options and might turn new users away.

You don't need tsfmt, just the default TypeScript settings that we ship with the tool

@octref
Copy link
Contributor Author

octref commented Oct 22, 2018

No, I use the default formatter options, e.g for TypeScript. Now prettifier makes TypeScript code look different then TypeScript makes it look and that's wrong if you ask me.

I also have different opinions on code style...I think the goal of a sample repo is consistency in code style, and prettier enforces the same style much better than TS formatter in my experience.

These are samples right? Why would anyone want to format them from the CLI?

This allows me to run prettier --write **/*.{ts,json} to enforce styles through CI.
I already ran it through the TS/json files we have: 23674b2 and 154ad38.

You don't need tsfmt, just the default TypeScript settings that we ship with the tool

The only way to enforce style in local setting (as user might have changed his user-level TS formatting setting), is to replicate the whole set of typescript.format.* settings in .vscode and all my-sample/.vscode.

@octref
Copy link
Contributor Author

octref commented Oct 24, 2018

@jrieken I dropped the formatter requirement for this milestone. Let's discuss later.
The new requirement is much more relaxed: no TS error and no TSLint error against these two rules:

{
	"rules": {
		"indent": [true, "tabs"],
		"semicolon": [true, "always"]
	}
}

I just did 3 LSP samples in 15 minutes. @Microsoft/vscode, if you are assigned for any samples, please take some time to update your samples. Otherwise, they will not end up in the listing on website and the README.

@jrieken
Copy link
Member

jrieken commented Oct 25, 2018

I just did 3 LSP samples in 15 minutes

my problem is that I ended up more or less rewriting the sample cos I didn't like it anymore... still, the outcome is an improvement.

@octref in the ## VS Code API section really just a listing of functions that are used? should this include all types that are being used from the API without further explanation?

@octref
Copy link
Contributor Author

octref commented Oct 25, 2018

my problem is that I ended up more or less rewriting the sample cos I didn't like it anymore... still, the outcome is an improvement.

The user would surely appreciate the update!

should this include all types that are being used from the API without further explanation?

I don’t want to list registerCommand for each sample. But if you do want to list all API calls that’s fine for me.

@dbaeumer
Copy link
Member

Shouldn't we clean the code as well and add the following compiler options?

		"strictNullChecks": true,
		"noImplicitAny": true,
		"noImplicitReturns": true,
		"noImplicitThis": true,
		"noUnusedLocals": true,
		"noUnusedParameters": true

@dbaeumer
Copy link
Member

I have added a tsconfig.base.json to the root of the samples repository.

@dbaeumer
Copy link
Member

Better base:

{
	"compilerOptions": {
		"strict": true,
		"noImplicitReturns": true,
		"noUnusedLocals": true,
		"noUnusedParameters": true
	}
}

@dbaeumer
Copy link
Member

In addition we should convert the tsc compile to use -b instead of -p especially if the example contains multiple tsconfig.json files.

@dbaeumer
Copy link
Member

Our source code should have the MS copyright. Currently missing in the hello world.

/* --------------------------------------------------------------------------------------------
 * Copyright (c) Microsoft Corporation. All rights reserved.
 * Licensed under the MIT License. See License.txt in the project root for license information.
 * ------------------------------------------------------------------------------------------ */

@dbaeumer
Copy link
Member

And we should remove "use strict"; from the TS files since it is emitted automatically if es6 targets are use or if --strict is used.

@Tyriar
Copy link
Member

Tyriar commented Oct 26, 2018

Our source code should have the MS copyright. Currently missing in the hello world.

@dbaeumer see #104 (comment)

@octref
Copy link
Contributor Author

octref commented Oct 26, 2018

Shouldn't we clean the code as well and add the following compiler options?

People would open each sub directory as an independent workspace, so I do hope each subfolder contains everything.

When we have microsoft/vscode-generator-code#114, people can generate an extension starter template using the samples too. They won't necessarily checkout the whole vscode-extension-samples.

In addition we should convert the tsc compile to use -b instead of -p especially if the example contains multiple tsconfig.json files.

We can do that for all the LSP samples and normalize them.

Shouldn't we clean the code as well and add the following compiler options?

The samples cater to a lot of people who might not be familiar with TS, so I hope the setting could be as relaxing as possible, so people can get started quickly instead of being bogged down by type errors.
I do agree we could clean up the code, though.

And we should remove "use strict";

I just did that for all samples.

Our source code should have the MS copyright.

Can I remove all of that too? We'll have a single top-level LICENSE / ThirdPartyNotice. I hope beginners can look at code and understand the ideas, not being reminded constantly all these code are owned by Microsoft.

@sandy081
Copy link
Member

sandy081 commented Nov 2, 2018

@octref Should we make vscode-extension-samples repo as a sample itself for MR workspace? IMO this is a perfect example as we are now enforcing same workspace settings for all samples.

I have already created a vscode-extension-samples.code-workspace file that has all samples with shared workspace settings.

@jrieken
Copy link
Member

jrieken commented Nov 2, 2018

I have already created a vscode-extension-samples.code-workspace file that has all samples with shared workspace settings.

📍 We have completed the circle 147c680

@octref
Copy link
Contributor Author

octref commented Nov 2, 2018

@sandy081 Please no, no one finds multi-root workspace with 20 roots useful. You type extension.ts and 20 suggestions show up. You search vscode. and 200 references from unrelated samples show up. No one wants to run yarn install or yarn watch or yarn compile for 20 projects that last minutes to finish.

The saddest part is then you find 5 more samples that's not shown in Quick Open or Search because people writing samples forgot to add them.

@sandy081
Copy link
Member

sandy081 commented Nov 5, 2018

If it was discussed before and decided to remove the MR workspace file, sorry as I am not aware.

Please no, no one finds multi-root workspace with 20 roots useful. You type extension.ts and 20 suggestions show up. You search vscode. and 200 references from unrelated samples show up.

@octref I think users have same issues when they open vscode-extension-samples root folder right? I think users who want to try sample(s) have following flow using VS Code.

  • Open VS Code
  • Git Clone vscode-extension-samples repo that offers an action to open the clone folder
  • Open vscode-extension-samples folder which shows all samples

Following are the disadvantages now

  1. There is no way to launch an example from here.
  2. If user wants to fix/enhance a sample and submit a PR, folder settings does not work.
  3. There is no quick way to open a specific example from here.

MR workspace will help with issues 1 & 2. Point 3 will be a good feature request for MR workspaces. Not sure if it makes sense to have this feature in single folder workspaces.

No one wants to run yarn install or yarn watch or yarn compile for 20 projects that last minutes to finish.

Not sure what you mean. How is MR workspace is related here?

The saddest part is then you find 5 more samples that's not shown in Quick Open or Search because people writing samples forgot to add them.

Is not it a general issue for MR workspace users? It would be great if VS Code helps them in such cases?

@octref
Copy link
Contributor Author

octref commented Nov 6, 2018

I should have added more instructions. The intended workflow is:

  • Clone the repo
  • Open any sample as its own, self-contained workspace
  • Develop in it

Here are some lists that never get updated:

https://github.com/Microsoft/vscode-extension-samples/blob/4524d2ae9d7c458baee22cc717e95c6ac98b0315/package.json#L6-L12
https://github.com/Microsoft/vscode-extension-samples/blob/4524d2ae9d7c458baee22cc717e95c6ac98b0315/.vscode/launch.json
https://github.com/Microsoft/vscode-extension-samples/blob/4524d2ae9d7c458baee22cc717e95c6ac98b0315/.vscode/tasks.json
https://github.com/Microsoft/vscode-extension-samples/blob/4524d2ae9d7c458baee22cc717e95c6ac98b0315/vscode-samples.code-workspace

Here's a postinstall script at root that tries to run npm install for each subfolder.
https://github.com/Microsoft/vscode-extension-samples/blob/4524d2ae9d7c458baee22cc717e95c6ac98b0315/.build/postinstall.js. It'll run forever.

There is no way to launch an example from here.

Showing 20 debug targets, and when user try to locate extension.ts, show them 20 files is not a good workflow.

If user wants to fix/enhance a sample and submit a PR, folder settings does not work.

Not really, git works as it is.
This actually ensures folder settings in each sample. See https://github.com/Microsoft/vscode-extension-samples/blob/master/.github/SAMPLE_GUIDELINE.md#3-vscode. One of my main problem used to be I have config for 2 space indentation, but whenever I develop vscode/extensions/css-language-features or any other features, I don't get the root indentation setting at vscode/.vscode/settings.json.

There is no quick way to open a specific example from here.

code <any-sample-folder>

Also there's my hope of making each sample a self-contained folder, able to serve as a starting point for making new extensions. See microsoft/vscode-generator-code#114.

@octref
Copy link
Contributor Author

octref commented Nov 6, 2018

In short, the hope is user finds a sample he's interested in and focus on it, not that he constantly get reminded he's in a MR workspace with 20 roots whenever he does Cmd+P, F5, Search, Open New Terminal, etc.

@alexdima
Copy link
Member

alexdima commented Nov 8, 2018

@octref

If we use tslint.json, then all extensions should have a clear devDependency to tslint. I don't install global node modules by principle and we should not assume that everyone has tslint installed globally.

@sandy081
Copy link
Member

sandy081 commented Nov 8, 2018

@octref After discussions I dropped the idea of making this MR workspace and make it single example specific.

@octref
Copy link
Contributor Author

octref commented Nov 8, 2018

@alexandrudima Good suggestion. Did that for all samples, except extension-deps-sample (no TS code) and hellocode-minimal-sample (in JS).

@octref octref modified the milestones: October 2018, November 2018 Nov 12, 2018
@DanTup
Copy link

DanTup commented Nov 13, 2018

Is it possible to add tests to the webpack example? I managed to get my extension working, but I can't figure out how to build/run the tests now. It should be possible to run the tests against the packed version of the extension (both from VS Code with F5 and on Travis etc.) but I'm not sure how.

@octref
Copy link
Contributor Author

octref commented Nov 13, 2018

@joaomoreno
Copy link
Member

@DanTup Can you give Azure Pipelines a try and let me know if you hit any problems with it?

@DanTup
Copy link

DanTup commented Nov 14, 2018

@octref @joaomoreno My issue isn't really about setting up CI, it's before that - I'm not sure how launch.json and scripts in package.json should be set up when using webpack. Copying the webpack example I end up removing the call to tsc in favour of calling webpack, which results in my extension being built (into dist/extension.js) but no tests being compiled.

It'd really help if the webpack sample was based on the normal hello_world extension that has some tests in it. (and also, it should be ready for running vsce package, but right now I'm not sure if it's including node_modules when it doesn't need to -> #119).

@joaomoreno
Copy link
Member

Maybe @jrieken has some insights.

jrieken added a commit that referenced this issue Nov 15, 2018
jrieken added a commit that referenced this issue Nov 15, 2018
@jrieken
Copy link
Member

jrieken commented Nov 15, 2018

While I am updating my sample I really question the use of TSLint. My samples are ~30 lines of code, a user open this and immediately sees an (annoying) prompt to install a recommended extension. Just so that after a restarting everything looks exactly like it did before.

What are we trying to achieve here? Yeah, I am friend of consistency but we are really throwing sticks at those people that wanna use the sample. First, we make them npm-install more than needed, then we make them install an extension which will only complain about the changes the user makes (since our stuff is already lint'ed). So, should using tabs and always having semicolons really be part of the learning experience?

I understand that we wanna provide "good looking" samples but now we just create PITA for those that use it. So we need to find a better way to enforce consistency (pre commit hook) and not force extensions and more config upon our users.

@octref
Copy link
Contributor Author

octref commented Nov 16, 2018

@jrieken I think a good middle ground is:

  • Let's include tslint.json and tslint in devDependencies
  • Yarn / NPM install will still be fast because of caching
  • Let's remove the TSLint workspace extension recommendation, so users don't get prompted for installing TSLint extension. But if they do install TSLint extension, linting errors will show up.

@octref
Copy link
Contributor Author

octref commented Jan 3, 2019

Shipped.

@octref octref closed this as completed Jan 3, 2019
@DanTup
Copy link

DanTup commented Jan 3, 2019

I posted above about adding tests to the webpack sample but that wasn't done - should I open a new issue about it? I still can't figure out how tests should be configured when packing an extension (see #110 (comment)).

@octref
Copy link
Contributor Author

octref commented Jan 3, 2019

@DanTup If microsoft/vscode-docs#2254 doesn't cover what you want, feel free to append to that issue or open a new one.

@DanTup
Copy link

DanTup commented Jan 3, 2019

@octref That issue seems relevant so I've added a comment on to there - thanks!

kna pushed a commit to kna/nadesiko3-vscode-extension that referenced this issue Nov 10, 2020
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

No branches or pull requests

9 participants