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

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

Merged
merged 6 commits into from Mar 26, 2024

Conversation

joemckenney
Copy link
Contributor

@joemckenney joemckenney commented Jan 10, 2024

+ 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 joemckenney changed the title Joe/update prettier feat: upgrade coc-prettier prettier version to 3.x.x Jan 10, 2024
@joemckenney
Copy link
Contributor Author

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)

@fannheyward
Copy link
Member

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

@fannheyward
Copy link
Member

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

@joemckenney
Copy link
Contributor Author

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

💯 - 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!

.prettierignore Outdated Show resolved Hide resolved
@joemckenney
Copy link
Contributor Author

joemckenney commented Jan 11, 2024

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. prettier/prettier-vscode#3007.

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

@fannheyward
Copy link
Member

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

@joemckenney
Copy link
Contributor Author

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
Copy link
Contributor Author

@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?

@fisker
Copy link

fisker commented Jan 11, 2024

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
Copy link

fisker commented Jan 11, 2024

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.

@joemckenney
Copy link
Contributor Author

joemckenney commented Jan 11, 2024

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.

@LumaKernel
Copy link
Contributor

LumaKernel commented Jan 28, 2024

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".

joemckenney and others added 2 commits January 28, 2024 15:23
fix: import issues +  formatting inconsistencies
@joemckenney
Copy link
Contributor Author

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

@joemckenney
Copy link
Contributor Author

@chemzqm any appetite for this change getting merged in?

@qauff
Copy link

qauff commented Mar 22, 2024

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

@joemckenney
Copy link
Contributor Author

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.

@LumaKernel
Copy link
Contributor

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

@qauff
Copy link

qauff commented Mar 25, 2024

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?

@fannheyward
Copy link
Member

@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.

@chemzqm chemzqm merged commit b0f67a8 into neoclide:master Mar 26, 2024
@qauff
Copy link

qauff commented Mar 27, 2024

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

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.

None yet

6 participants