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?]: TS error in preview.tsx & main.ts when using absolute (src/-prefixed) and/or component directory imports [storybook-vite] #11741

Closed
1 task
Philzen opened this issue Dec 3, 2024 · 17 comments · Fixed by #11746
Labels
bug/needs-info More information is needed for reproduction

Comments

@Philzen
Copy link
Contributor

Philzen commented Dec 3, 2024

What's not working?

As reported in the forum during the storybook migration from webpack to vite:

screenshot

Above squiggles amount to Cannot find module '{import path}' or its corresponding type declarations.. Storybook compiles fine though with not complaints whatsoever.

@arimendelow

Current workaround

Use relative imports and specify the full component path (i.e. ../src/components/ConfirmDialog/ConfirmDialog works for the above example).

How do we reproduce the bug?

  1. Utilize cli-storybook-vite and edit main.ts or preview.tsx in web/.storybook
  2. Try adding an absolute path to any component starting with src/ or any path to a component that only refers to the component directory (i.e. ../src/components/MyComponent instead of ../src/components/MyComponent/MyComponent)

What's your environment? (If it applies)

System:
    OS: Linux 6.12 Manjaro Linux
    Shell: 5.2.37 - /bin/bash
  Binaries:
    Node: 20.16.0 - /tmp/xfs-88cf9d03/node
    Yarn: 4.4.0 - /tmp/xfs-88cf9d03/yarn
  Databases:
    SQLite: 3.46.1 - /usr/bin/sqlite3
  npmPackages:
    @redwoodjs/auth-dbauth-setup: 8.4.1 => 8.4.1 
    @redwoodjs/cli-storybook-vite: 8.4.1 => 8.4.1 
    @redwoodjs/core: 8.4.1 => 8.4.1 
    @redwoodjs/project-config: 8.4.1 => 8.4.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 Dec 3, 2024
@arimendelow
Copy link
Contributor

This is an issue that touches codegen that I am not familiar with — there's a bunch of magic that happens when you eg run yarn rw g types to make the component directory import work. Issue is, it only applies to things within the src directory. There's also things going on with tsconfig, which TBH is also something I'm not so knowledgable of 🙃

Is this breaking anything? The syntactic sugar of being able to use an absolute import is certainly nice, but enabling it here may not be worth the investment.

@Tobbe, can you weigh in here?

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

You can add .storybook folder to .tsconfig like:

  "include": [
    "src",
    "config",
    "./.storybook/**/*",
    "../.redwood/types/includes/all-*",
    "../.redwood/types/includes/web-*",
    "./types"
  ]

This allows to use relative imports

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

My problem with storybook it's that mockCurrentUser just doesn't work, even with the provider as decorator

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

Also, eslint does not work for this files with current eslint config

@Tobbe
Copy link
Member

Tobbe commented Dec 5, 2024

This is an issue that touches codegen that I am not familiar with — there's a bunch of magic that happens when you eg run yarn rw g types to make the component directory import work. Issue is, it only applies to things within the src directory. There's also things going on with tsconfig, which TBH is also something I'm not so knowledgable of 🙃

Is this breaking anything? The syntactic sugar of being able to use an absolute import is certainly nice, but enabling it here may not be worth the investment.

@Tobbe, can you weigh in here?

I've never actually worked with Storybook, so I feel a bit outside of my comfort zone with this one 😬
But I can certainly take a look if get more detailed reproduction steps than what is provided in the original post. Like, if I start with yarn create redwood-app --yes rw-sb-ts-error what do I do next?

@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

what do I do next?

Generate a component, then open up web/.storybook/preview.tsx and then try to import that component. You should find that only relative imports using the full path work, so it's like:

  • ✔️ import MyComponent from ../src/components/MyComponent/MyComponent`

vs. what you would expect to work (as it does everywhere else in the RW stack)

  • import MyComponent from '../src/components/MyComponent'
  • import MyComponent from 'src/components/MyComponent'
  • import MyComponent from 'src/components/MyComponent/MyComponent'

Note that all of these are purely "cosmetical" – the code compiles and storybook starts up fine, however VS Code is generating red sqiggles (interestingly enough, neither rw lint nor rw tc will flag those imports)


Noteworthy:

  • import * as React from 'react' is also seen as required by VS Code, while removing it is perfectly fine, storybook starts up
  • VS Code also seems to struggle with auto-importing components as i type them out. What i mean is normally everywhere in the RW project i can start typing <MyComponent then hit CTRL+Space which will bring up options for importing. However in any files in the src/.storybook folder it won't find anything to suggest.

So summing up this is only a QOL / DX bug, not a big showstopper. But it looks like it may be easily fixable, as it feels like somewhere the storybook config path simply needs to be included.

@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

You can add .storybook folder to .tsconfig like:

  "include": [
    "src",
    "config",
    "./.storybook/**/*",
    "../.redwood/types/includes/all-*",
    "../.redwood/types/includes/web-*",
    "./types"
  ]

This allows to use relative imports

@irg1008 i think this nails it, cheers

I guess during the move from from web/config to web/.storybook this was simply forgotten to be updated.

This solves all issues above 🥳

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

export function useAuth() {

Is this mocked useAuth being used in storybook right now?

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

useAuth not being mocked up when inside storybook

About this. I fixed it. I think I need to point out how and what I did wrong.

For storybook auth mock we use:
https://github.com/redwoodjs/redwood/blob/c367143b23a11a852bbfdd914970f6982fc4b6c2/packages/storybook/src/plugins/mock-auth.ts

In the script we can see the path detected for transformation is

if (id.includes('web/src/auth')) {

When moving our auth.ts file from root folder to let's say "lib" folder, storybook is not able to find the file and therefore replace it with the mock.

  • web/src/auth.ts => OK
  • web/src/lib/auth.ts => Not Ok

It's an issue with me since I change the file location but could we provide a way to set auth file location or maybe add some common paths like 'lib/auth' or similar?

@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

export function useAuth() {

Is this mocked useAuth being used in storybook right now?

@irg1008 Apart from this being slightly off-topic in this issue here – i'm not sure what you mean. Shouldn't we simply use mockCurrentUser in Storybook?

@irg1008
Copy link
Contributor

irg1008 commented Dec 5, 2024

Yeah sorry about that, I meant that the location of auth.ts file on the web side it's mandatory to be on the root folder. Otherwise the storybook plugin won't pick it up and mock it, making mockCurrentUser function to use the original unmocked function

@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

Yeah sorry about that, I meant that the location of auth.ts file on the web side it's mandatory to be on the root folder. Otherwise the storybook plugin won't pick it up and mock it, making mockCurrentUser function to use the original unmocked function

Ah i see, thanks for the explanation. I guess that would be defined somewhere in the @redwoodjs/cli-storybook-vite package's code. But i believe it's best to open up a separate issue (could propably be an RFC) to discuss this.

@Philzen
Copy link
Contributor Author

Philzen commented Dec 5, 2024

Also, eslint does not work for this files with current eslint config

Ah yeah that's another gotcha. I solved it by adding "ignorePatterns": ["!.storybook/"] to my local extended eslint config. However i believe that should be fixed in "@redwoodjs/eslint-config" as well.

I've now created #11748 to rectify that as well.

@Tobbe
Copy link
Member

Tobbe commented Dec 5, 2024

When moving our auth.ts file from root folder to let's say "lib" folder, storybook is not able to find the file and therefore replace it with the mock.

  • web/src/auth.ts => OK
  • web/src/lib/auth.ts => Not Ok

It's an issue with me since I change the file location but could we provide a way to set auth file location or maybe add some common paths like 'lib/auth' or similar?

I wouldn't be surprised if this also breaks other things. We probably assume the location of your auth file is at web/src/auth.ts in more places (But maybe we're lucky and we don't!)

Anyway, if we can make this configurable for Storybook I'm all for that 🙂

@arimendelow
Copy link
Contributor

You can add .storybook folder to .tsconfig like:

  "include": [
    "src",
    "config",
    "./.storybook/**/*",
    "../.redwood/types/includes/all-*",
    "../.redwood/types/includes/web-*",
    "./types"
  ]

This allows to use relative imports

@irg1008 i think this nails it, cheers

I guess during the move from from web/config to web/.storybook this was simply forgotten to be updated.

This solves all issues above 🥳

Oh yay!!!! Yup, that's my mistake :) I'm still more comfortable using the framework and still a little newer to building the framework.

@arimendelow
Copy link
Contributor

When moving our auth.ts file from root folder to let's say "lib" folder, storybook is not able to find the file and therefore replace it with the mock.

  • web/src/auth.ts => OK
  • web/src/lib/auth.ts => Not Ok

It's an issue with me since I change the file location but could we provide a way to set auth file location or maybe add some common paths like 'lib/auth' or similar?

I wouldn't be surprised if this also breaks other things. We probably assume the location of your auth file is at web/src/auth.ts in more places (But maybe we're lucky and we don't!)

Anyway, if we can make this configurable for Storybook I'm all for that 🙂

Ditto on this :) Didn't even occur to me that anyone would want to move that. @irg1008 what was your reason for moving it? And have you run into any other issues related to it?

@irg1008
Copy link
Contributor

irg1008 commented Dec 8, 2024

When moving our auth.ts file from root folder to let's say "lib" folder, storybook is not able to find the file and therefore replace it with the mock.

  • web/src/auth.ts => OK
  • web/src/lib/auth.ts => Not Ok

It's an issue with me since I change the file location but could we provide a way to set auth file location or maybe add some common paths like 'lib/auth' or similar?

I wouldn't be surprised if this also breaks other things. We probably assume the location of your auth file is at web/src/auth.ts in more places (But maybe we're lucky and we don't!)

Anyway, if we can make this configurable for Storybook I'm all for that 🙂

Ditto on this :) Didn't even occur to me that anyone would want to move that. @irg1008 what was your reason for moving it? And have you run into any other issues related to it?

None, I thought moving this file didn't mean anything, so I moved it to lib to match the api folder structure somehow.

I think that if we don't have file based routing, we shouldn't have to have this constraint for specific file locations

Or maybe some sort of way to customize the paths.

Anyway it's not big deal, just moved it back to the root folder

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
None yet
Development

Successfully merging a pull request may close this issue.

4 participants