coc-prettier icon indicating copy to clipboard operation
coc-prettier copied to clipboard

feat: upgrade coc-prettier prettier version to 3.x.x

Open joemckenney opened this issue 1 year ago • 14 comments

+ Update hand-coded prettier types + Refactor code consuming prettier methods which are now async

Note: the formatting is mixed in this repo so I'm not entirely sure how to avoid unwanted formatting getting into this PR given that i use coc-prettier -- which is why I'm making this PR in the first place.

joemckenney avatar Jan 10 '24 04:01 joemckenney

Two remaining items

  • I need to update the code that watches for config changes to look for the newly accepted config files
  • I need to come up with a way of testing this locally (probably via local modules)

joemckenney avatar Jan 10 '24 04:01 joemckenney

It's better to follow this repo's quote style, don't change to double quotes for better reviews.

fannheyward avatar Jan 10 '24 04:01 fannheyward

come up with a way of testing this locally

  1. :CocUninstall coc-prettiter from your list
  2. set runtimepath^=~/path/to/your/local/coc-prettier in your vimrc

fannheyward avatar Jan 10 '24 04:01 fannheyward

It's better to follow this repo's quote style, don't change to double quotes for better reviews.

:100: - the issue is that this repo has mixed formatting e.g., ModuleResolve.ts uses double quotes whereas PrettierEditService.ts uses single quotes.

I'll just disable formatting in my editor while working on this repo.

  1. :CocUninstall coc-prettiter from your list
  2. set runtimepath^=~/path/to/your/local/coc-prettier in your vimrc

Thanks for the pointer!

joemckenney avatar Jan 10 '24 16:01 joemckenney

Some findings from testing.

When running coc-prettier with this branch's build enabled, I get the following error.

[coc.nvim]: UnhandledRejection: A dynamic import callback was not specified.                                                                                                                                                                 
TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING]: A dynamic import callback was not specified.                                                                                                                                             
    at new NodeError (node:internal/errors:372:5)                                                                                                                                                                                            
    at importModuleDynamicallyCallback (node:internal/process/esm_loader:39:9)                                                                                                                                                               
    at Object.<anonymous> (/home/joe/repos/forks/coc-prettier/node_modules/prettier/index.cjs:593:23)                                                                                                                                        
    at Module.<anonymous> (/home/joe/.vim/plugged/coc.nvim/build/index.js:80325:28)                                                                                                                                                          
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)                                                                                                                                                              
    at Module.load (node:internal/modules/cjs/loader:981:32)                                                                                                                                                                                 
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)                                                                                                                                                                       
    at Module.require (node:internal/modules/cjs/loader:1005:19)                                                                                                                                                                             
    at req (/home/joe/.vim/plugged/coc.nvim/build/index.js:80309:17)                                                                                                                                                                         
    at Object.<anonymous> (/home/joe/repos/forks/coc-prettier/lib/index.js:3935:24)    

The error originates from this line in the prettier source code.

This seems to be a problem other editor plugins have run into e.g. https://github.com/prettier/prettier-vscode/issues/3007.

So, concretely, when loading the plugin via this code, we get an error due to how prettier is doing its dynamic imports.

joemckenney avatar Jan 11 '24 00:01 joemckenney

Thank you for your PR and debugging, will test this ASAP, I don't use prettier much...

fannheyward avatar Jan 11 '24 01:01 fannheyward

Thank you for your PR and debugging, will test this ASAP, I don't use prettier much...

My best guess is the way we are loading the plugin in here is incompatible with ESM modules? I had trouble understanding whether the vm.runInContext was ESM compatible or not, though the errors would suggest no.

joemckenney avatar Jan 11 '24 02:01 joemckenney

@fisker any input from the prettier side about what we might be doing wrong here? Should we be using a different export of prettier that would work around this issue?

joemckenney avatar Jan 11 '24 17:01 joemckenney

I'm not familiar with this codebase, after a quick look, it seems we use vm to load prettier, there are some esm support issues in vm, but I'm not very sure what exactly they are.

this jest issue may be helpfully.

If it's possible, I suggest move to pure ESM.

fisker avatar Jan 11 '24 21:01 fisker

BTW, I saw this "A dynamic import callback was not specified." in our tests (JEST) a lot when migrate prettier to ESM. We end up make tests passes with a native ESM environment.

fisker avatar Jan 11 '24 21:01 fisker

I see, so basically, the usage of vm.runInContext in coc.nvim is the issue.

So this change/branch won't work unless upstream changes in coc.nvim were made to support loading dynamic imports.

joemckenney avatar Jan 11 '24 22:01 joemckenney

https://github.com/LumaKernel/coc-prettier_fork_joe/tree/luma

compare to this PR (treat my changes under CC0)

I fixed this a little (dynamic import problem, fix trailingComma default in package.json), and it's running well in my project (using type=module and prettier v3).

https://prettier.io/blog/2023/07/05/3.0.0.html

Additionally, the default value of trailingComma has been changed to "all".

LumaKernel avatar Jan 28 '24 18:01 LumaKernel

@chemzqm I believe this is ready for review. I've tested that it is working locally.

joemckenney avatar Jan 29 '24 00:01 joemckenney

@chemzqm any appetite for this change getting merged in?

joemckenney avatar Feb 05 '24 16:02 joemckenney

Any updates on merging and releasing this? I'm finding I'm having this issue too.

englut avatar Mar 22 '24 23:03 englut

Any updates on merging and releasing this? I'm finding I'm having this issue too.

This change getting merged is blocked by someone with write access to this repo merging it. I pinged @chemzqm a number of times regarding this.

joemckenney avatar Mar 25 '24 17:03 joemckenney

I guess he concerns backward-compatibility. We have another way, just publishing coc-prettier3

LumaKernel avatar Mar 25 '24 17:03 LumaKernel

I guess he concerns backward-compatibility. We have another way, just publishing coc-prettier3

Ah. that's great, is there a rough estimate of when that might be?

englut avatar Mar 25 '24 19:03 englut

@joemckenney @qauff @LumaKernel thank you for your PR!

This feature has been released in coc-prettier-dev, try :CocUninstall coc-prettier and :CocInstall coc-prettier-dev.

https://www.npmjs.com/package/coc-prettier-dev

Note: coc-prettier-dev is only used for unmerged PRs, and after PR merged into coc-prettier, you should use coc-prettier instead.

fannheyward avatar Mar 26 '24 01:03 fannheyward

Thanks for merging @chemzqm @fannheyward ! Will there be a 9.3.3 release soon?

englut avatar Mar 27 '24 23:03 englut