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

Go To Definition [of Cell (in VSCode)] takes you to index.d.ts #2867

Closed
ajoslin103 opened this issue Jun 21, 2021 · 2 comments · Fixed by #9269
Closed

Go To Definition [of Cell (in VSCode)] takes you to index.d.ts #2867

ajoslin103 opened this issue Jun 21, 2021 · 2 comments · Fixed by #9269
Labels

Comments

@ajoslin103
Copy link
Contributor

Go To Definition [of Cell (in VSCode)] takes you to the [mirrored] index.d.ts in the .redwood folder

I have no idea what to do, other than report this

(same result for Go To Definition, and Go To Type Definition)

lord knows this is not a show-stopper

more like an oddity

@thedavidprice
Copy link
Contributor

@peterp thoughts on this one?

@jtoar
Copy link
Contributor

jtoar commented Jun 28, 2021

We thought we fixed this in #2614 (we closed an issue similar to this one: #669 (comment)). Definitely still an issue though, I've reproduced it

@jtoar jtoar added the bug/confirmed We have confirmed this is a bug label Jun 28, 2021
@jtoar jtoar removed the v1/priority label May 6, 2022
Josh-Walker-GM added a commit that referenced this issue Oct 10, 2023
…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
jtoar pushed a commit that referenced this issue Oct 10, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants