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

Add TypeScript types #35

Merged
merged 10 commits into from
Jun 8, 2020
Merged

Add TypeScript types #35

merged 10 commits into from
Jun 8, 2020

Conversation

remcohaszing
Copy link
Member

This add TypeScript types for the following packages:

  • rehype
  • rehype-parse
  • rehype-stringify

Also Prettier has been properly configured. It runs on the entire code base. Markdown and HTML files are ignored. Because of this, many JSON files have been touched.

@remcohaszing remcohaszing changed the title Add TypeScrtipt types Add TypeScript types May 8, 2020
@ChristianMurphy ChristianMurphy requested a review from a team May 8, 2020 16:05
@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🦋 type/enhancement This is great to have 🧑 semver/major This is a change labels May 8, 2020
@ChristianMurphy
Copy link
Member

/cc @Rokt33r

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @remcohaszing!

packages/rehype-parse/types/index.d.ts Outdated Show resolved Hide resolved
packages/rehype-parse/types/index.d.ts Outdated Show resolved Hide resolved
@ChristianMurphy ChristianMurphy requested a review from a team May 8, 2020 16:38
@wooorm
Copy link
Member

wooorm commented May 10, 2020

I’d prefer not formatting JSON either, what do y’all think?
JSON for us is either tree fixtures or package.json, they’re generated and modified by tools instead of written by hand, and their main role is to be read in git diffs, for which a more stable format like JSON.stringify would be less noisy?

packages/rehype-stringify/package.json Show resolved Hide resolved
packages/rehype/package.json Outdated Show resolved Hide resolved
@remcohaszing
Copy link
Member Author

remcohaszing commented May 10, 2020

@wooorm wrote:

I’d prefer not formatting JSON either, what do y’all think?
JSON for us is either tree fixtures or package.json, they’re generated and modified by tools instead of written by hand, and their main role is to be read in git diffs, for which a more stable format like JSON.stringify would be less noisy?

I think Prettier should be used to format JSON, because it forces consistent formatting and very importantly, proper spacing, newlines, and a newline at the end of the file.

Prettier supports 2 formatters for JSON: json and json-stringify. By default all JSON files are formatted using their json formatter, except the ones listed here

@wooorm’s preferred format matches json-stringify. Prettier can be configured to enforce this style instead using the following option:

"overrides": [
      {
        "files": [
          "*.json"
        ],
        "options": {
          "parser": "json-stringify"
        }
      }
    ]

Although I personally like json-stringify better as well, I also like how Prettier is about giving up your preference of coding style for a consistent one enforced by a tool.

Note that the Prettier json format is actually jsonc, so it supports comments.

@wooorm
Copy link
Member

wooorm commented May 16, 2020

Ah. Yeah. Overrides sounds silly.
Still feel like proper spacing, newlines, and a end-of-file newline is mostly done automatically. You did catch one missing eof eol, and I don’t like them either, but in my opinion it also doesn’t hurt that much.
/cc @ChristianMurphy, @Murderlon, what do you think?

@Murderlon
Copy link
Member

First off, nice work @remcohaszing!

/cc @ChristianMurphy, @Murderlon, what do you think?

I don’t think it matters a lot but I might be slightly inclined to not using Prettier on JSON as well. Mostly because the files, like @wooorm said, are fixtures or package.json. Future formatting updates to Prettier might create diff noise, or require a new PR to be made to update the fixtures.

@ChristianMurphy
Copy link
Member

I don't have a strong opinion on the json formatting. 🤷
I skip json formatting in most of my personal prettier setups.

If this config were getting bundled into a unified-dx tool which could apply the override to every project it may be a good idea. 👍
Trying to remember to add the override to every project manually sounds like a recipe for confusion and inconsistency 😅

@remcohaszing
Copy link
Member Author

I think running Prettier on the entire directory, instead of using a glob is the correct way of running it. Files are excluded using .prettierignore. This way, running prettier . yields the same results as saving supported project files in the editor.

I reconfigured Prettier, because I needed it to format TypeScript files as well. I decided to format JSON as well, as the impact isn’t that big. Markdown is excluded, because remark-preset-wooorm conflicts with prettier, and HTML files are excluded, because these fixtures are sometimes malformed on purpose for the sake of testing Rehype.

All file of these file formats are formatted by Prettier. Excluding a specific file extension in .prettierignore is more configuration just formatting the file. Also for example .travis.yml is formatted. It just happens to conform to the prettier formatting.

Even if files are generated, people can mess with them before committing them, so I think it makes sense to validate any file if possible, especially if a file type is already supported by a tool that’s already in use.

If you do have strong feelings about the JSON formatting, I’ll exclude it from syntax-tree/hast-util-from-parse5#10 and later this PR. Otherwise, I’ll wait on that PR, since this one depends on it.

@wooorm
Copy link
Member

wooorm commented May 18, 2020

I, relatively strongly, would say that we should do a ‘.’ for prettier always, but would like to exclude through prettierignore: 1) gitignored files when needed, 2) markdown because we dog food our own, 3) HTML because it’s probably a fixture when needed, 4) json (if included). Everything else, yaml, TS, JS, etc., is probably handwritten and really benefits from formatting.
But this preference is for the projects I originally authored, and there may be exceptions per project.

I agree on formatting as important, but don’t think the churn of updating 100s of mostly generated files is worth it.

@ChristianMurphy
Copy link
Member

friendly ping @remcohaszing

@codecov-commenter

This comment has been minimized.

@remcohaszing
Copy link
Member Author

@ChristianMurphy sorry, I have been busy unexpectedly lately. Anyway, the feedback is processed now. :)

@wooorm wooorm merged commit 38bdb01 into rehypejs:master Jun 8, 2020
@wooorm
Copy link
Member

wooorm commented Jun 8, 2020

Released, thanks @remcohaszing and others!

@wooorm wooorm added the 💪 phase/solved Post is done label Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

6 participants