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

Configuring pre-commit for jsx, Typescript, tsx #2745

Closed
silouanwright opened this issue Sep 4, 2017 · 7 comments
Closed

Configuring pre-commit for jsx, Typescript, tsx #2745

silouanwright opened this issue Sep 4, 2017 · 7 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Comments

@silouanwright
Copy link

silouanwright commented Sep 4, 2017

There's not a lot of documentation on how to use the pre-commit method (maybe that's by design?) but this is what I've done.

pip install pre-commit
touch .pre-commit-config.yaml

.pre-commit-config.yaml

    -   repo: https://github.com/prettier/prettier
        sha: 'a29b407'  # Use the sha or tag you want to point at
        hooks:
        -   id: prettier
pre-commit install
pre-commit autoupdate

.App.jsx

    export default class extends Component {
        handleChange = (e) => {
            this.setState({
                input: e.target.value,
            });
            console.log('aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah');
            console.log('aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah');
            console.log('aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah');
            console.log('aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah', 'aye', 'foo', 'blah', 'blah');
    
            [
                        'lorem',
                        'ipsum',
                        'dolor',
                        sit('amet'),
                        consectetur['adipiscing'] + 'elit',
                    ].reduce((first, second) => first + second, '');
    
        }
        // ...
        // ....
git add --all
git commit

Then I see this:

screen shot 2017-09-03 at 8 05 39 pm

This is working on regular js files if I attempt to commit them, but not jsx or tsx (I didn't try regular ts).

As to why I'm using pre-commit, it's in the Readme as being acceptable to use, and I'd also like to keep the ability to stage hunks without staging the entire file.

I also have a followup question: does the pre-commit stuff use the .prettierrc config file, or do you need to pass in your own styles? If so, how do you do this as well?

@silouanwright silouanwright changed the title Trying to use Pre-commit: prettier says there are no files to check? Configuring pre-commit for jsx, ts, tsx Sep 4, 2017
@silouanwright silouanwright changed the title Configuring pre-commit for jsx, ts, tsx Configuring pre-commit for jsx, Typescript, tsx Sep 4, 2017
@chriskuehl
Copy link

chriskuehl commented Sep 5, 2017

I think it's because by default the hooks only target javascript and not jsx:

types: [javascript]

from identify, jsx is identified as "jsx" and not "javascript":

https://github.com/chriskuehl/identify/blob/db0d8cc8a2b53345d747031aa4e21a5dcb125818/identify/extensions.py#L60

You could overwrite that with types: [jsx], but then it won't target javascript. Unfortunately for this case, listing both is an AND and not an OR, so you can't just list jsx and javascript. Maybe @asottile has an idea of how best to solve this?

@asottile
Copy link
Contributor

asottile commented Sep 5, 2017

pre-commit maintainer here!

When I set up the integration here I didn't know enough about prettier it seems :) -- prettier handles more filetypes than just .js files!

One option would be to change the prettier .pre-commit-hooks.yaml file to more generically list the file extensions of what it supports, something like:

-    types: [javascript]
+    files: \.(js|jsx|ts|tsx)$

this unfortunately would lose linting of extensionless files as well... maybe this is ok for prettier? imo this is probably the best solution for the short term.

Another idea you could implement just with your own configuration would be to duplicately use the prettier hook, but locally configure it for the other file types you want to lint:

-   repo: https://github.com/prettier/prettier
     sha: 'a29b407'  # Use the sha or tag you want to point at
     hooks:
     -   id: prettier
         name: prettier (js)
     -   id: prettier
         name: prettier (jsx)
         # override the default `types: [javascript]`
         types: [jsx]
     -   id: prettier
         name: prettier (typescript)
         # identify doesn't yet know about ts / tsx files, a pull request would be great! https://github.com/chriskuehl/identify
         # reset the `types` detection to the default
         types: [file]
         files: \.(ts|tsx)$

As for the configuration file, I'm not sure of prettier's behaviour, but I believe it should use whatever configuration is in the current working directory when executed -- pre-commit makes sure to execute hooks from the root of the git repository.

@vjeux
Copy link
Contributor

vjeux commented Sep 5, 2017

Those are all the file types prettier handles by default:

prettier/src/options.js

Lines 32 to 42 in 133303f

if (/\.(css|less|scss)$/.test(filepath)) {
normalized.parser = "postcss";
} else if (/\.html$/.test(filepath)) {
normalized.parser = "parse5";
} else if (/\.(ts|tsx)$/.test(filepath)) {
normalized.parser = "typescript";
} else if (/\.(graphql|gql)$/.test(filepath)) {
normalized.parser = "graphql";
} else if (/\.json$/.test(filepath)) {
normalized.parser = "json";
}

@asottile when you did the integration, prettier only handled js and jsx, we expanded a bit the scope of prettier since then ;)

@asottile
Copy link
Contributor

asottile commented Sep 5, 2017

Awesome, should we approach this by listing extensions supported by prettier in the configuration (via files:)? I think that's probably the most flexible way forward -- we can revisit this when I invent some way to have an OR relationship with types.

@vjeux
Copy link
Contributor

vjeux commented Sep 5, 2017

@asottile whatever works for you, I'll be happy to merge a PR to update the config :)

@asottile
Copy link
Contributor

asottile commented Sep 5, 2017

Sweet, here's my proposal in PR form: #2759

@asottile
Copy link
Contributor

asottile commented Sep 5, 2017

@reywright the latest configuration (just merged) should support ts / tsx as well -- thanks for the issue!

To get the latest version (before a tagged release is made in prettier) you can use pre-commit autoupdate --bleeding-edge (this will get the latest revision of master instead of the latest tag). Or use 4d68faf or newer.

@chriskuehl thanks again for pinging :D
@vjeux if there's additional issues about pre-commit integration now or in the future, feel free to ping me :)

@azz azz closed this as completed Sep 6, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests

5 participants