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

Suggested fixes to tsconfig, prettier config #156

Merged
merged 5 commits into from
Dec 22, 2022

Conversation

elliot-nelson
Copy link
Contributor

SUMMARY

  • Move prettier config into Prettier's dedicated config file, for ease of discovery
  • Add the prettier-plugin-packagejson prettier plugin
  • Turn off useUnknownInCatchVariables typescript rule

DETAILS

I am open to pushback on this, but I think in general, using package.json as an unstructured grab-bag is less discoverable than using tool-specific configuration files. So I was hoping to split out .prettierrc.js into its own file in the root.

It would also be nice to add prettier-plugin-packagejson -- it's very opinionated, as it wraps the opinionated sort-package-json utility, but I've found it really nice to just give in and let VSCode auto-sort my whole packagejson every time I touch it.

Last, I think we (and everyone else) should turn off useUnknownInCatchVariables. Notably, even the TypeScript codebase itself turns this off -- the extra safety is not worth the hassle until TS starts offering some better assumptions (like at least being able to specify err: Error).

@changeset-bot
Copy link

changeset-bot bot commented Dec 20, 2022

⚠️ No Changeset found

Latest commit: 1d8392b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

tsconfig.json Outdated Show resolved Hide resolved
Comment on lines -38 to -40
"prettier": {
"proseWrap": "never"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm in the "explicit configs camp" too 👍

.prettierrc.js Outdated
Comment on lines 11 to 12
// Disabling search dirs and pointing directly at folder paths relative to this config file
// makes it easier for VSCode and WebStorm to find your plugins.
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting - do you happen to have a link to some issues related to this? I'm just curious why the automatic plugin search tends to fail in those contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you've challenged me on it, I suspect that this is a defense mechanism for Rush in particular.

If you have a Lerna repo or something and you use a root-level package.json to keep your prettier devDependency, which lives in a root-level node_modules, then I think 99.9% of the time VSCode would "do the right thing".

But in a Rush repo, there is no root package or node modules, and plugins are kept in a special folder; VSCode typically can't find this folder unless you specify exactly where it is. I can test out simplifying this for this repo since it probably would work just fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, ye - this might makes sense in the context of Rush if the location of plugins etc is not standard. That being said, I prefer an explicit list of plugins rather than one based on auto-search.

.prettierrc.js Outdated Show resolved Hide resolved
Co-authored-by: Mateusz Burzyński <[email protected]>
@elliot-nelson
Copy link
Contributor Author

^ One note -- I don't know if this is normal or not -- but it looks like this PR doesn't run CI checks at all (I'd feel a bit safer if it was actually running CI on a change like this).

@Andarist
Copy link
Collaborator

^ One note -- I don't know if this is normal or not -- but it looks like this PR doesn't run CI checks at all (I'd feel a bit safer if it was actually running CI on a change like this).

It's the whole mining prevention measure implemented by GitHub. For first-time contributors, the CI needs a maintainer's approval to run. I've approved this workflow - it should pass in a moment, I will land this and you won't have that problem again.

@Andarist Andarist merged commit 9237533 into Thinkmill:main Dec 22, 2022
@elliot-nelson elliot-nelson deleted the enelson/devx branch December 22, 2022 14:28
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

Successfully merging this pull request may close these issues.

2 participants