fix(types): Generate maps to allow improved "go to definition" behaviour #9269
Conversation
|
@Josh-Walker-GM this is super, really. This is been such a long standing problem, its great that you've found a solution.🚀🚀🚀🚀, very impressed. Two thoughts:
|
|
@dac09 I think all pages in |
|
Thanks @dac09, glad it helps! Answering in reverse order:
I think there is potentially more we can do with this technique to address even more misdirections that aren't addressed in this PR directly. |
Oh good point @Tobbe! I hadn't thought about that one, I'll fix that too in this PR. |
Refactors the implementation too.
|
I should note that this is one possible solution. It's a reasonably small and quick change which improved the behaviour for me in VSCode. A more robust solution would be to implement an LSP/VSCode extension which can provide these go-to-definition instructions - this would be more generic and extendable to things like cell auto-completion. That however is a task with a much larger scope and time commitment. |
There was a problem hiding this comment.
Having done a quick scan at the code, looks pretty OK to me!
Observations from a quick test:
✅ Routes navigating correctly
🟡 Navigating to Cell working, but jumping to top of the file (can see why from the code). Does jumping to the success component here make sense or is it extra logic we dont really need?
✅ Other cmd+click seems to be intact
Happy for you to merge as is though :)
|
Discussed with the team and it makes the most sense to direct cells to the |
…our (#9269) **Problem** Navigating to the definition of pages, layouts, components, cells etc. would lead you to the .d.ts file. This can be really frustrating if you're used to flying around between definitions this way. **Changes** 1. Generates basic definition/source mappings for: 1. directory mapped modules 2. cell mirrors 3. router pages 4. route links **Linked issues** Fixes #5862 Fixes #2867 **Performance** This adds additional AST parsing work to the `yarn rw g types` so it has the potential to slow that command down. I have tested that command on the test project and the results are: Benchmark: `hyperfine --runs 32 --warmup 3 'yarn rw g types'` Main: ``` Benchmark 1: yarn rw g types Time (mean ± σ): 4.006 s ± 0.048 s [User: 5.244 s, System: 0.737 s] Range (min … max): 3.926 s … 4.171 s 32 runs ``` PR: ``` Benchmark 1: yarn rw g types Time (mean ± σ): 4.629 s ± 0.049 s [User: 6.066 s, System: 0.828 s] Range (min … max): 4.544 s … 4.723 s 32 runs ``` An approximate +15% change in duration on this basic test. There are areas where caching results could improve performance. For example we are likely finding the location of page components twice when we could cache the first result. I have not added this here so we do not prematurely optimise and introduce subtle bugs that would be more difficult to track down. If users with large projects report problems with this performance change then we can work to address it. **Notes** This is not an area I'm particularly knowledgeable about so there could be unknown consequences to this that I don't know about. For pages and other web components it's easy to direct the map to the definition of the specific component that is the default export. This is not the case for cells as they have no default export and instead have a number of other non-default exports. In this case I currently direct the map to the head of the source file - happy to improve on this based on feedback. For future reference I found the following useful: * https://www.murzwin.com/base64vlq.html * https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.1ce2c87bpj24
Problem
Navigating to the definition of pages, layouts, components, cells etc. would lead you to the .d.ts file. This can be really frustrating if you're used to flying around between definitions this way.
Changes
Linked issues
Fixes #5862
Fixes #2867
Performance
This adds additional AST parsing work to the
yarn rw g typesso it has the potential to slow that command down. I have tested that command on the test project and the results are:Benchmark:
hyperfine --runs 32 --warmup 3 'yarn rw g types'Main:
PR:
An approximate +15% change in duration on this basic test.
There are areas where caching results could improve performance. For example we are likely finding the location of page components twice when we could cache the first result. I have not added this here so we do not prematurely optimise and introduce subtle bugs that would be more difficult to track down. If users with large projects report problems with this performance change then we can work to address it.
Notes
This is not an area I'm particularly knowledgeable about so there could be unknown consequences to this that I don't know about.
For pages and other web components it's easy to direct the map to the definition of the specific component that is the default export. This is not the case for cells as they have no default export and instead have a number of other non-default exports. In this case I currently direct the map to the head of the source file - happy to improve on this based on feedback.
For future reference I found the following useful: