feat(rsc): custom scope analyzer to replace periscopic#1157
feat(rsc): custom scope analyzer to replace periscopic#1157james-elicx wants to merge 9 commits intovitejs:mainfrom
Conversation
|
Hi @hi-ogawa, sorry to be a pain - I was wondering if you might have an idea of when you'll get chance to do an initial review? |
|
Sorry for stalling. Will try to take a look this week 🙏 |
| } | ||
|
|
||
| currentScope = fnBodyScope ?? programScope.scope | ||
| walk(fnNode.body, { |
There was a problem hiding this comment.
Is it possible to avoid walking again? What I was hoping is to make initial analysis richer so that we can check each reference inside "use server"'s exact declaration. For example, I think perisopic's issue was references: string[] being raw identifier string, but what if each reference keeps actual declaration Node?
There was a problem hiding this comment.
Thanks, this was good feedback. I've spent the past couple of hours re-working it a bit to track references in the initial pass instead of needing the re-walk. I do think that the analysis phase seems much more valuable / useful for callers after implementing more detailed reference tracking there. Overall, much better outcome IMO. The func capture tracking is then just based on the references that were found.
As I was going through it I also had an idea of an edge case for self-referencing functions that are actually references involving shadowed functions, so included a fix for that too.
Interested to hear what you think of the new approach.
|
Hey, thanks for pushing this and sorry for taking time. I've been reviewing and gone quite a rabbit hole with various implementation and ended up designing scope manager from ground up in #1170. I'd appreciate if you can review 🙏 |
No worries, thanks for letting me know - if you want to continue with that one instead I don't mind closing this PR. I copied over my test cases into a clone of your branch and it looks like a bunch of them are failing for self-referencing functions and binding member expressions rather than raw objects, which are two of the issues my original PRs before this one were intending to fix, so it wouldn't resolve all the issues this PR is at the moment. Happy to review, although it might be worth copying the same tests over and fixing those first as that may change what it looks like |
|
Binding member expressions is planned as follow-up in #1172 as it looks like it's worth defining the problem scope property first and thus require its own design decision. I also left out self-reference case as it seemed like an edge case. Please let me know if that one comes out in practice. |
|
I've released a new version 0.5.22 which includes shadowing fix #1171 and member based binding #1172. Some test cases in your PR were incorporated through #1175 Thanks again for the contribution. I'm going to close this PR since the approach is superseded. Recursive action support in this PR might be still a useful reference and we can add it separately when we find a need. Also please let me know if the new release works for Vinest users and I'd appreicate the feeback. Thanks! |
2 participants
Description
This PR implements our own scope analyzer to replace periscopic.
Pulled in the test cases from both #1151 and #1155, and generated several more, to ensure that it works for several different edge cases.
The new analyzer gives the following benefits:
varto the function body.I used AI to help with ensuring we were getting the right the behaviour and logic we wanted, but a large chunk of the implementation was written by myself, and the bits that weren't were refactored.
Busy Sunday!
Close #1151
Close #1155