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

[WIP] feat($plugins): start the work on supporting custom plugins #120

Conversation

andreiglingeanu
Copy link
Contributor

Summary

This PR implements the work described in #119. We start to depend on custom known plugins and provide config for them out of the box.

For now, this contains almost everything that is needed for that to work, besides the --plugins flag passing to prettier binary. I'm not really sure how to approach that, because we need to gracefully fallback to a user-defined .prettierrc file.

Test Plan

  1. Open a *.php ,*.swift or *.py file and run :Prettier

@andreiglingeanu
Copy link
Contributor Author

Also, we should really warn Swift users to follow prerequisites, otherwise there is a risk for a lot of confusion. There should be a strategy for this case...

@mitermayer
Copy link
Member

mitermayer commented Mar 11, 2018

This is awesome!! thank you for your work @andreiglingeanu.

Also, we should really warn Swift users to follow prerequisites, otherwise there is a risk for a lot of confusion. There should be a strategy for this case...

I think maybe could add some documentation around it on both doc/prettier.txt as well as on README, feel free to submit on a separate PR if you prefer.

Before I merge this would be awesome to add some more info for this plugins support on the README and on the documentation doc/prettier.txt what are your thoughts around it ? If you prefer to submit a separate PR for it that could work as well.

Once again thank you very much for this PR, can't wait to use this when developing in python and php and maybe in a near future swift.

Copy link
Member

@mitermayer mitermayer left a comment

Choose a reason for hiding this comment

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

This PR looks good to me, I wonder if we should pin to stable release versions of the plugins on our package.json, other than that we should update the readme and docs and it should be good to be merged!

Thanks a lot!

@andreiglingeanu
Copy link
Contributor Author

One more important thing that should be figured out is how to gracefully detect and fallback the case when the user defines plugins config entry in the .prettierrc file.

We should somehow default to our node_modules directory from ~/.vim/bundle/vim-prettier/node_modules. Also, this path should be computed dynamically, because on neovim it becomes ~/.config/nvim/bundle/.

@mitermayer
Copy link
Member

mitermayer commented Mar 11, 2018

Hi @andreiglingeanu,

One more important thing that should be figured out is how to gracefully detect and fallback the case when the user defines plugins config entry in the .prettierrc file.

Do you mind explaining a bit more that scenario ? We already allow user to overwrite any defaults since we use config-precedence option of prettier (https://github.com/prettier/vim-prettier/blob/master/autoload/prettier.vim#L310). That means any .prettierrc file or package.json config will be used to overwrite our defaults

We should somehow default to our node_modules directory from ~/.vim/bundle/vim-prettier/node_modules. Also, this path should be computed dynamically, because on neovim it becomes ~/.config/nvim/bundle/.

We already do that (https://github.com/prettier/vim-prettier/blob/master/autoload/prettier.vim#L361), but that has the assumption that the user has done npm install or yarn install inside the plugin directory. If they installed vim-prettier following our README by:

" post install (yarn install | npm install) then load plugin only for editing supported files
Plug 'prettier/vim-prettier', {
  \ 'do': 'yarn install',
  \ 'for': ['javascript', 'typescript', 'css', 'less', 'scss', 'json', 'graphql', 'markdown', 'vue'] }

The post install hook of vim-plug will run yarn install on the vim-prettier directory regardless of where neovim or vim have it defined.


If I misunderstood what were your concerns were do you mind giving an example ?

@mitermayer
Copy link
Member

Please keep me posted around the above, so that we can merge this PR soon

@andreiglingeanu
Copy link
Contributor Author

For some reason this PR, as it is, is not enough for me to correctly format a PHP file. It might be that prettier is not picking up plugins from our package.json. I've checked with :PrettierCliPath -- we do use prettier binary from vim-prettier/node_modules/.bin/prettier

Can you reproduce that one on your end?

@mitermayer
Copy link
Member

Will try this PR locally and see what I can find

@mitermayer
Copy link
Member

Hi @andreiglingeanu ,

I have just tested your PR and it worked for me when having the 'plugin' field defined on the '.prettierrc' or on package.json.

I am going to patch this PR to also use the default one when the user does not have it installed. Will hopefuly have sometime to work on this tomorrow or the day after and will merge this

@andreiglingeanu
Copy link
Contributor Author

Sounds good, looking forward to it. Thank you!

@mitermayer
Copy link
Member

Another thing I noticed while testing was that :Prettier sync was working for the PHP plugin, however the async version was causing vim to hang.

Will investigate this a bit more. To not block this PR what we could do is partially enable plugin support in the meantime by forcing it to run synchronous while we investigate that issue.

What I will do is create a holding branch for this issues and merge this PR in there so that I can do some more tweaks on it before publishing to master

@andreiglingeanu
Copy link
Contributor Author

That's definitely very strange. Is the PHP plugin the one who hangs around?

@mitermayer
Copy link
Member

Still haven't had time to get back into this, what i will do is create a new branch 1.0.x for vim-prettier to start merging those PRS and fix things there in preparation for 1.0. release

@mitermayer
Copy link
Member

That should also allow some users to use that branch to help finding issues

@mitermayer mitermayer mentioned this pull request Mar 25, 2018
13 tasks
@mitermayer mitermayer added this to Backlog in vim-prettier 1.0 May 16, 2018
@gigo6000
Copy link

@mitermayer @andreiglingeanu I would love to test this, but I can't find this changes in the 1.x branch

@mitermayer
Copy link
Member

Hi @gigo6000,

We have not added this yet to 1.x due to some prOblems we encountered earlier. Hopefully this weekend I will have enought time to do so!

@mitermayer
Copy link
Member

Will get back to this one this week, this is one of the last issues that we would like to be solved before 1.x

@andreiglingeanu
Copy link
Contributor Author

Hey @mitermayer, I think I can squeeze some time this week to push through getting this thing forward. Is there anything I can help you with?

@gigo6000
Copy link

gigo6000 commented Aug 2, 2018

I'm not sure why this hasn't been merged but if anyone is interested in using Prettier with PHP feel free to use my fork in the meantime: https://github.com/gigo6000/vim-prettier

@mitermayer
Copy link
Member

This has not been merged yet, apologies! Been swamped at work as I’m going on parental leave soon (2nd September) but getting back to this ASAP

@gigo6000
Copy link

gigo6000 commented Aug 2, 2018

@mitermayer no worries, this is open source :). Congrats on your baby!

@andreiglingeanu
Copy link
Contributor Author

Congrats on your baby!

@mitermayer
Copy link
Member

This has been merged on the release/1.x branch #155

@mitermayer mitermayer closed this Nov 5, 2018
@czosel
Copy link

czosel commented Nov 6, 2018

Just tested it, works flawlessly! 👍 I can't wait to update the README of prettier-php once 1.x has been released 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants