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

[Bug?]: Code navigation to a cell implementation not possible (ends up in type definition) #5862

Closed
1 task
Philzen opened this issue Jul 1, 2022 · 3 comments · Fixed by #9269
Closed
1 task
Assignees
Labels
bug/needs-info More information is needed for reproduction

Comments

@Philzen
Copy link
Contributor

Philzen commented Jul 1, 2022

What's not working?

As a developer i'd like to be able to CTRL+click on any react component to pull up its implementation code.

This works fine in VSCode for everything but cells, likely caused by the fact that they don't have a matching default export.

Related

How do we reproduce the bug?

  1. Generate a Cell, let's say MyCell
  2. import and use it in any other component (e.g. a page)
  3. Hold CTRL while clicking on <MyCell … /> in your IDE of choice

The IDE opens .redwood/types/mirror/web/src/components/MyCell/index.d.ts instead of web/src/components/MyCell/MyCell.tsx.

What's your environment? (If it applies)

System:
    OS: Linux 5.15 Manjaro Linux
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 16.15.1 - /tmp/xfs-c559b1ba/node
    Yarn: 3.2.0 - /tmp/xfs-c559b1ba/yarn
  Databases:
    SQLite: 3.38.5 - /usr/bin/sqlite3
  Browsers:
    Firefox: 102.0
  npmPackages:
    @redwoodjs/core: ^1.5.1 => 1.5.1

Are you interested in working on this?

  • I'm interested in working on this
@Philzen Philzen added the bug/needs-info More information is needed for reproduction label Jul 1, 2022
@dac09
Copy link
Collaborator

dac09 commented Jul 1, 2022

Hi, closing this as its a dupe of #5097

I'm not aware of any solutions for this at this time, but will be very happy if there are any suggestions!

@dac09 dac09 closed this as completed Jul 1, 2022
@Philzen
Copy link
Contributor Author

Philzen commented Jul 2, 2022

I'm not aware of any solutions for this at this time, but will be very happy if there are any suggestions!

@dac09 i think already you were already on to something in your tape that you posted over at #4303

closing this as its a dupe of #5097

I was worried about that at first before i opened this ticket, that's why i made some tests with WebStorm and these are clearly two separate issues 🧐 – although they are in the same domain (webpack, typemapping, default exports and stuff).

#5097 is about Components not resolving correctly in IDEs other than VSCode when using the "short" import path – yes that is all components, Cells are actually not mentioned in that issue.

That's why i opened this issue to specifically look at the challenge with cells, which are a different beast because there is no default export that would reference the cell itself. If at all, this could be considered a duplicate of #4303, and that ticket's scope should be widened – but the code navigation issue (this ticket) which may or may not be solved when #4303 is solved (depending on how it is solved). Therefore it may make sense to keep track of the different aspects separately.

@jtoar jtoar reopened this Jul 5, 2022
@Tobbe
Copy link
Member

Tobbe commented Jul 5, 2022

The solution that I hinted at over in #4303 was specific to Cells IIRC. Unfortunately I prototyped the solution in GitPod, and it was lost in a browser crash a long time ago 🙁

If my memory serves me right the idea was to generate a file somewhere inside .redwood/ that imported the Cell in a way that "educates" VSCode about its existence.

At the time I had no idea about the complexities involved with keeping generated files in sync with actual files. But now I'm painfully aware of it all due to #5820

After #5820 is merged I, or someone else, can try to implement my now lost solution again, and see if it still holds up.

@Tobbe Tobbe self-assigned this Jul 5, 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
bug/needs-info More information is needed for reproduction
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants