-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
created a vscode workspace file for the repo #29830
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
6eace6a
to
0912d74
Compare
Comparing: 814a418...76dc6e8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
c0e22f5
to
9ba27d0
Compare
9ba27d0
to
9ca627b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in person, VSCode users should have a better devx, hopefully
Oftentimes repos commit a So I usually prefer if repos add a If users have their own workspace config they couldn't benefit from the Though the settings.template.json requires an additional step. We could get fancy and automatically copy in |
@eps1lon This is another reason I like the
I'd also encourage that |
@eps1lon I see the benefits of using the I think going with the workspace approach would be a bit better for now until microsoft/vscode#15909 lands. |
Summary: While reviewing facebook/react#29830, I noticed this file was committed with tab indentation in React Native. Note: The `json-stringify` parser can be used with Prettier 3+ only, so we use `json` instead. Changelog: [Internal] Differential Revision: D58413581
…nore Summary: While reviewing facebook/react#29830, I noticed this file was committed with tab indentation in React Native. I have also used the `.gitignore` entry to clarify how `react-native.code-workspace` interacts with an optional user `.vscode/` config directory. Note: The `json-stringify` parser can be used with Prettier 3+ only, so we use `json` instead. Changelog: [Internal] Differential Revision: D58413581
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. However, people are pretty opinionated about their workflows. We already have linting and prettier setup to ensure consistency, i'm not sure this is providing clear value.
We can discuss at our next contributor's sync, but I'm not sure we want to move forward with this.
Summary: Pull Request resolved: #44874 While reviewing facebook/react#29830, I noticed this file was committed with tab indentation in React Native. I have also used the `.gitignore` entry to clarify how `react-native.code-workspace` interacts with an optional user `.vscode/` config directory. Note: The `json-stringify` parser can be used with Prettier 3+ only, so we use `json` instead. Changelog: [Internal] Reviewed By: vzaidman Differential Revision: D58413581 fbshipit-source-id: 58c14db6648fed10736062b1f055475154aa74a4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you leave this PR only containing changes for react.code-workspace
? There's a whole bunch of other unrelated changes
925f1dd
to
64d81be
Compare
@poteto just did. Thanks! |
{ | ||
"folders": [ | ||
{ | ||
"path": "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to opt out /compiler
? We use TypeScript there not Flow so these would be the wrong set of settings to apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix this (approximating) by scoping the javascript.validate.enable
setting by file type. (Do test this!)
// Does not match `.ts`, `.tsx`, approximating all flow files
"[javascript]": {
"javascript.validate.enable": false
},
All other settings are innocuous under compiler/
.
Edit: I just realised javascript.validate.enable
and typescript.validate.enable
are separate — this should all be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed with:
"prettier.configPath": "",
"prettier.ignorePath": ""
that would ensure that the local setting of prettier is used to validate the files inside "compiler" as opposed to the default settings that always relies on the config file in the root.
this way prettier is used for formatting and picks up typescript in the compiler folder
react.code-workspace
Outdated
"editor.formatOnSave": true, | ||
"flow.pathToFlow": "${workspaceFolder}/node_modules/.bin/flow", | ||
"javascript.validate.enable": false, | ||
"search.useIgnoreFiles": true, | ||
"search.exclude": { | ||
"**/dist/**": true, | ||
"**/build/**": true, | ||
"**/out/**": true, | ||
"*.map": true, | ||
"*.log": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how many of these are strictly necessary to getting started with the repo? I think only flow.pathToFlow
and javascript.validate.enable
are needed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I just checked and
search.useIgnoreFiles
can be removed indeed. search.exclude
is an important one because it enables search with "files to include" to ignore these paths. For some weird reason VSCode doesn't respect.gitignore
when "files to include" is used, but does respect it when searching without "files to include".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would still be good to limit the workspace to exclude the compiler directory, but I guess that doesn't need to block this PR
64d81be
to
9acc323
Compare
9acc323
to
2974dd4
Compare
2974dd4
to
307947c
Compare
307947c
to
76dc6e8
Compare
fixed- the compiler directory is now verified just fine! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing our discussion in the design sync:
- The main concern with this is if we could end up introducing tooling (for contributors) that relies on a specific editor setup. We all use varying setups, and if code formatting or testing or whatever only worked in one editor, that would be not good.
- A second concern is that people have their own workflows even with VSCode and may not want to use this.
- The primary motivation is that it took @vzaidman a while to set up their editor to be productive, and we want onboarding to be as painless as possible (the problem domain is complex enough, we don't incidental complexity of tooling setup).
Since the main concern is theoretical, we'll keep an eye on it. The secondary concern is alleviated by this being optional. Let's ship and see how it goes.
Summary
Similarly to what has been done on the
react-native
repo in facebook/react-native#43851, this PR adds areact.code-workspace
workspace file when using VSCode.This disables the built-in TypeScript Language Service for
.js
,.ts
, and.json
files, recommends extensions, enablesformatOnSave
, excludes certain files in search, and configures Flow language support.Motivation
This is a DevX benefit for React contributors using open source VS Code. Without this, it takes quite a long time to set up the environment in vscode to work well.
For me the following two points took around an hour each to figure out, but for others it may take even more (screenshots can be found below):
Recommended extensions
NOTE: The recommended extensions list is currently minimal — happy to extend this now or in future, but let's aim to keep these conservative at the moment.
.editorconfig
, all file typesWhy
react.code-workspace
?.code-workspace
files have slight extra behaviours over a.vscode/
directory:https://code.visualstudio.com/docs/editor/workspaces
Test plan
Against a new un-configured copy of Visual Studio Code Insiders.
Without workspace config
❌ .js files raise errors by default (built-in TypeScript language service)
❌ When using the Flow VS Code extension, the wrong version (global) of Flow is used.
❌ Searching in excluded files when the "include" field is specified
With workspace config
✅ Built-in TypeScript Language Service is disabled for .js files, but still enabled for .ts[x] files
✅ Flow language support is configured correctly against flow version in package.json
✅ Does not search in excluded files when the "include" field is specified
✅ Workspace config is suggested when folder is opened in VS Code
✅ Dialog is shown on workspace launch with recommended VS Code extensions