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

created a vscode workspace file for the repo #29830

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

vzaidman
Copy link
Contributor

@vzaidman vzaidman commented Jun 10, 2024

Summary

Similarly to what has been done on the react-native repo in facebook/react-native#43851, this PR adds a react.code-workspace workspace file when using VSCode.

This disables the built-in TypeScript Language Service for .js, .ts, and .json files, recommends extensions, enables formatOnSave, 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):

  • Search with "files to include" was searching in ignored files (compiled/generated)
  • Configure language validation and prettier both in "packages" that use flow and in the "compiler" folder that uses typescript.

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.

  • Flow — language support
  • EditorConfig — formatting based on .editorconfig, all file types
  • Prettier — formatting for JS* files
  • ESLint — linter for JS* files

Why react.code-workspace?

.code-workspace files have slight extra behaviours over a .vscode/ directory:

  • Allows user to opt-in or skip.
  • Allows double-click launching from file managers.
  • Allows base folder (and any subfolders in future) to be opened with local file tree scope (useful in fbsource!)
  • (Minor point) Single config file over multiple files.
    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.
Screenshot 2024-06-10 at 16 03 59

❌ Searching in excluded files when the "include" field is specified
Screenshot 2024-06-10 at 15 41 24

With workspace config

✅ Built-in TypeScript Language Service is disabled for .js files, but still enabled for .ts[x] files
Screen Recording 2024-06-13 at 12 21 24

✅ Flow language support is configured correctly against flow version in package.json
Screenshot 2024-06-10 at 16 03 44

✅ Does not search in excluded files when the "include" field is specified
Screenshot 2024-06-10 at 15 39 18

✅ Workspace config is suggested when folder is opened in VS Code
image

✅ Dialog is shown on workspace launch with recommended VS Code extensions
Screenshot 2024-06-10 at 15 40 52

@vzaidman vzaidman requested review from huntie and hoxyq June 10, 2024 14:43
Copy link

vercel bot commented Jun 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2024 1:33pm

@react-sizebot
Copy link

react-sizebot commented Jun 10, 2024

Comparing: 814a418...76dc6e8

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.80 kB 497.80 kB = 89.24 kB 89.24 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.62 kB 502.62 kB = 89.94 kB 89.94 kB
facebook-www/ReactDOM-prod.classic.js = 597.04 kB 597.04 kB = 105.31 kB 105.31 kB
facebook-www/ReactDOM-prod.modern.js = 571.38 kB 571.38 kB = 101.25 kB 101.25 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 76dc6e8

Copy link
Contributor

@hoxyq hoxyq left a 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

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 11, 2024

Oftentimes repos commit a .vscode/settings.json but that's annoying if you want to customize your folder settings until microsoft/vscode#15909 lands.

So I usually prefer if repos add a .vscode/settings.example.json with instructions to cp .vscode/settings.example.json .vscode/settings.json after clone. DT does that: https://github.com/DEfinitelyTyped/DefinitelyTyped/#are-files-formatted-automatically

If users have their own workspace config they couldn't benefit from the .code-workspace when they could benefit from a settings.template.json

Though the settings.template.json requires an additional step. We could get fancy and automatically copy in postinstall if a .vscode/settings.json doesn't exist yet.

@huntie
Copy link
Member

huntie commented Jun 11, 2024

@eps1lon This is another reason I like the <repo>.code-workspace pattern. A user-defined .vscode/ directory (which we should add to .gitignore) allows exactly this kind of override, for merged per-user customisations over these defaults.

Image: The font size preference of 16 is applied by .vscode/settings.json, taking precedence over the workspace file.

image

I'd also encourage that react.code-workspace defines as many base settings as possible, to share between all users.

@vzaidman
Copy link
Contributor Author

@eps1lon I see the benefits of using the .vscode/settings.example.json approach, however even with a postinstall I don't like how there's no nice way to update the settings automatically for users.

I think going with the workspace approach would be a bit better for now until microsoft/vscode#15909 lands.

huntie added a commit to huntie/react-native that referenced this pull request Jun 11, 2024
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
huntie added a commit to huntie/react-native that referenced this pull request Jun 11, 2024
…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
Copy link
Contributor

@josephsavona josephsavona left a 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.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 11, 2024
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
Copy link
Member

@poteto poteto left a 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

@vzaidman
Copy link
Contributor Author

@poteto just did. Thanks!

{
"folders": [
{
"path": "."
Copy link
Member

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

Copy link
Contributor Author

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!

Copy link
Member

@huntie huntie Jun 13, 2024

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.

Copy link
Contributor Author

@vzaidman vzaidman Jun 13, 2024

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

now it works:
Screen Recording 2024-06-13 at 12 21 24

Comment on lines 16 to 21
"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
Copy link
Member

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?

Copy link
Contributor Author

@vzaidman vzaidman Jun 12, 2024

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".
    image

Copy link
Member

Choose a reason for hiding this comment

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

ah gotcha

Copy link
Member

@poteto poteto left a 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

@vzaidman
Copy link
Contributor Author

vzaidman commented Jun 13, 2024

@poteto

Would still be good to limit the workspace to exclude the compiler directory, but I guess that doesn't need to block this PR

fixed- the compiler directory is now verified just fine!

Copy link
Member

@poteto poteto left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@josephsavona josephsavona left a 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.

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.

8 participants