-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
Two remaining items
|
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.
Thanks for the pointer! |
1a01bcb
to
abf62f8
Compare
Some findings from testing. When running
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. |
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. |
@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? |
I'm not familiar with this codebase, after a quick look, it seems we use this jest issue may be helpfully. If it's possible, I suggest move to pure ESM. |
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. |
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. |
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
|
fix: import issues + formatting inconsistencies
@chemzqm I believe this is ready for review. I've tested that it is working locally. |
@chemzqm any appetite for this change getting merged in? |
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. |
I guess he concerns backward-compatibility. |
Ah. that's great, is there a rough estimate of when that might be? |
@joemckenney @qauff @LumaKernel thank you for your PR! This feature has been released in 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. |
Thanks for merging @chemzqm @fannheyward ! Will there be a 9.3.3 release soon? |
+ 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.