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 to prettier 3 #2200

Merged
merged 8 commits into from
Aug 16, 2023
Merged

feat: upgrade to prettier 3 #2200

merged 8 commits into from
Aug 16, 2023

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Aug 12, 2023

No description provided.

@czosel czosel mentioned this pull request Aug 12, 2023
@czosel
Copy link
Collaborator Author

czosel commented Aug 12, 2023

This is WIP, and any help is appreciated. I'll print some of the open issues, since for some reason they don't show up in CI.

 FAIL   test-node  tests/enum/jsfmt.spec.js                                                                                                                                            
  ● enum.php                                                                                                                                                                           
                                                                                                                                                                                       
    TypeError: Cannot convert undefined or null to object                                                                                                                              
        at Function.keys (<anonymous>)                                                                                                                                                 
                                                                                                                                                                                       
      903 |                                                                                                                                                                            
      904 |   const getChildNodes = (node) =>                                                                                                                                          
    > 905 |     Object.keys(node)                                                                                                                                                      
          |            ^                                                                                                                                                               
      906 |       .filter(                                                                                                                                                             
      907 |         (n) =>                                                                                                                                                             
      908 |           n !== "kind" &&                                                                                                                                                  
                                                                                                                                                                                       
      at keys (src/comments.js:905:12)                                                                                                                                                 
      at getChildNodes (src/comments.js:920:10)                                                                                                                                        
      at getSortedChildNodes (node_modules/prettier/index.mjs:17203:64)                                                                                                                
      at node_modules/prettier/index.mjs:17208:52                                                                                                                                      
          at Array.flatMap (<anonymous>)                                                                                                                                               
      at getSortedChildNodes (node_modules/prettier/index.mjs:17207:6)                                                                                                                 
      at decorateComment (node_modules/prettier/index.mjs:17220:22)                                                                                                                    
      at decorateComment (node_modules/prettier/index.mjs:17231:14)                                                                                                                    
      at node_modules/prettier/index.mjs:17287:8                                                                                                                                       
          at Array.map (<anonymous>)                                                                                                                                                   
      at attachComments (node_modules/prettier/index.mjs:17286:38)                                                                                                                     
      at prepareToPrint (node_modules/prettier/index.mjs:17787:3)                                                                                                                      
      at printAstToDoc (node_modules/prettier/index.mjs:17721:20)                                                                                                                      
      at coreFormat (node_modules/prettier/index.mjs:18037:20)                                                                                                                         
      at formatWithCursor (node_modules/prettier/index.mjs:18235:14)                                                                                                                   
      at format (tests_config/run_spec.js:128:18)                                                                                                                                      
      at Object.<anonymous> (tests_config/run_spec.js:71:22) 
 FAIL   test-node  tests/class/jsfmt.spec.js                                                                                                                                           
  ● anonymous.php                                                                                                                                                                      
                                                                                                                                                                                       
    TypeError: Invalid value used as weak map key                                                                                                                                      
        at WeakMap.set (<anonymous>)                                                                                                                                                   

      126 |
      127 | async function format(source, filename, options) {
    > 128 |   const result = await prettier.formatWithCursor(
          |                  ^
      129 |     source,
      130 |     Object.assign({ filepath: filename }, options)
      131 |   );

      at getSortedChildNodes (node_modules/prettier/index.mjs:17213:19)
      at node_modules/prettier/index.mjs:17208:52
          at Array.flatMap (<anonymous>)
      at getSortedChildNodes (node_modules/prettier/index.mjs:17207:6)
      at decorateComment (node_modules/prettier/index.mjs:17220:22)
      at decorateComment (node_modules/prettier/index.mjs:17231:14)
      at decorateComment (node_modules/prettier/index.mjs:17231:14)
      at node_modules/prettier/index.mjs:17287:8
          at Array.map (<anonymous>)
      at attachComments (node_modules/prettier/index.mjs:17286:38)
      at prepareToPrint (node_modules/prettier/index.mjs:17787:3)
      at printAstToDoc (node_modules/prettier/index.mjs:17721:20)
      at coreFormat (node_modules/prettier/index.mjs:18037:20)
      at formatWithCursor (node_modules/prettier/index.mjs:18235:14)
      at format (tests_config/run_spec.js:128:18)
      at Object.<anonymous> (tests_config/run_spec.js:71:22)

@czosel
Copy link
Collaborator Author

czosel commented Aug 12, 2023

I was able to narrow it down a bit - the issues come from getCommentChildNodes(node), which seems to behave differently to prettier v2.

@fisker Any pointers are greatly appreciated 🙂

src/comments.js Outdated Show resolved Hide resolved
src/comments.js Outdated Show resolved Hide resolved
src/comments.js Outdated Show resolved Hide resolved
@fisker
Copy link
Member

fisker commented Aug 14, 2023

Unless we still want support Prettier v2, I'd suggest use new property getVisitorKeys instead of getCommentChildNodes since we are not handling special cases inside.

const ignoredKeys = new Set(["kind", "loc", "errors", "extra"]);

function getVisitorKeys(node, nonTraversableKeys) {
  return Object.keys(node).filter(
    (key) => !nonTraversableKeys.has(key) && !ignoredKeys.has(key),
  );
}

https://prettier.io/docs/en/plugins#optional-getvisitorkeys

@czosel
Copy link
Collaborator Author

czosel commented Aug 14, 2023

Thank you @fisker! getVisitorKeys works much better than our previous implementation. We're down to 3 failing tests now 👍 (some detail about comment handling seems to have changed as well, I don't look into it yet)

@czosel
Copy link
Collaborator Author

czosel commented Aug 14, 2023

Update: It seems that the way followingNode is determined for comments slightly changed in Prettier 3. E.g. for the following input

<?php
$a = new class (
    // 1
    $a,
    // 2
    $b
) {
  const foo = "bar";
};

followingNode of // 1 is

  • the Variable node $a for Prettier 2
  • the classconstant node (const foo = "bar";) for Prettier 3

For this example, this looks like a regression - but I didn't check what exactly changed upstream yet and why. @fisker any pointers are much appreciated, as usual 😉

@fisker
Copy link
Member

fisker commented Aug 15, 2023

I took a quick look.

When attaching // 1, Prettier found class is the "enclosingNode", and it's children doesn't include variable[name="a"] or variable[name="b"] (since they belong to new, the AST looks strange to me), so it's attached to the only child classconstant.

Not sure how it works in Prettier v2 yet, but it won't be easy to fix.

I haven't used PHP for a long time. $a and $b are arguments of the "new call", not parameters of the class?

@fisker
Copy link
Member

fisker commented Aug 16, 2023

@czosel I pushed an ugly workaround, seems the result is acceptable. 8cfceb9 Not sure if there are missing cases since I'm not familiar with theASTand syntax.

@czosel czosel changed the title feat: upgrade to prettier 3 (wip) feat: upgrade to prettier 3 Aug 16, 2023
@czosel
Copy link
Collaborator Author

czosel commented Aug 16, 2023

Thanks so much @fisker, this looks great! 💯 The new class is not super common in PHP anyway, so the hack seems fine to me.

@czosel czosel merged commit 49bf194 into prettier:main Aug 16, 2023
12 checks passed
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

2 participants