Core: Add ./ to start of hidden file & folder paths

[IanVS]

Issue:

I'm trying to troubleshoot storybookjs/builder-vite#155, where I'm having trouble with stories that are defined inside of my .storybook folder. I found that storybook only adds a ./ to the start of a filename if it doesn't already find a single . at the start of the file, which inadvertently picks up hidden files & folders.

What I did

I've created a helper function to add the ./ when necessary, so it can be shared between the two places we know of where the same thing is done. I used @joshwooding's suggested snippet, because I couldn't think of anything better and I think it's pretty solid.

How to test

yarn test lib/core-common/src/utils/__tests__/paths.test.ts
yarn test lib/core-common/src/utils/__tests__/to-importFn.test.ts
yarn test lib/core-common/src/utils/__tests__/normalize-stories.test.ts

• Is this testable with Jest or Chromatic screenshots?
• Does this need a new example in the kitchen sink apps?
• Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

[nx-cloud]

Nx Cloud Report

CI ran the following commands for commit a580de0. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	nx run-many --target=prepare --all --parallel --max-parallel=15

---

Sent with 💌 from NxCloud.

[tmeasday]

I like where this is going @IanVS. LMK if you need more help from me.

[joshwooding]

Seems to me the bug is here

storybook/lib/core-common/src/utils/normalize-stories.ts

Lines 51 to 53 in 5da733d

	if (!directoryFromWorking.startsWith('.')) {
	directoryFromWorking = `.${path.sep}${directoryFromWorking}`;
	}

I don't know if there's a nice way to write it but we need something to essentially do:

  if (!(directoryFromWorking.startsWith(`.${path.sep}`) || directoryFromWorking.startsWith(`..${path.sep}`))) {
    directoryFromWorking = `.${path.sep}${directoryFromWorking}`;
  }

[IanVS]

Good catch, I think there are a few place spread around where this happens. There's also

storybook/lib/core-server/src/utils/StoryIndexGenerator.ts

Line 106 in 177dd45

const importPath = slash(relativePath[0] === '.' ? relativePath : `./${relativePath}`);

, which I think is what determines what "fileName" is sent to the client for a given story id.

[IanVS]

@tmeasday I've created a helper function to add the ./ when necessary, so it can be shared between the two places we know of where the same thing is done. I used @joshwooding's snippet, because I couldn't think of anything better and I think it's pretty solid.

I've also split the workingDir and configDir in the tests as you suggested, but I have a failing case that I'm not sure whether it's a real problem or just due to the test setup.

The tests that I wrote for the ./ issue are now passing, at least. I can't see the CircleCI results for the tests, and I can't seem to run them locally successfully. LMK if there's something else I can do here.

[IanVS]

lib/store/src/autoTitle.test.ts seems to be failing because windows path separators are used, but the test is running on linux, so path.sep isn't matching. Any ideas what to do about that?

[shilman]

@IanVS we use the slash library to normalize windows paths. if there's a windows test case that's failing, it's a legitimate failure and points to a missing slash in the code

[IanVS]

@IanVS we use the slash library to normalize windows paths. if there's a windows test case that's failing, it's a legitimate failure and points to a missing slash in the code

Thanks, but I think the problem is that the test is faking a windows path by using \\, but since the code under test uses path.sep, it doesn't match (since the test is running on Linux). Is that possible?

[shilman]

@IanVS unless those tests are new, in theory if it was passing before it means that the contract is that the code under test should be able to handle windows paths as input and did before this PR. our tests are a mess, so i'm not confident that theory matches practice in this case 🤷‍♂️

[IanVS]

@shilman previously the code only looked for a . at the start of the path. Now with the fix in this PR, it looks for a .${path.sep}. We'd have to run the windows test on a windows box for it to be a good test. Maybe to accommodate the test I should use a regex instead, and check for either a / or \.

[shilman]

Works for me! 👍

[joshwooding]

Using regex would cover all cases :) and you can consolidate the check into one regex

[tmeasday]

Really great stuff @IanVS. I would like to test it on a windows box (unless you did already?) -- let me do that before we merge it.

[IanVS]

I've updated the check to use regex as discussed, and the windows-style path tests are now passing. I also added some windows-style path tests to my new function as well.

What is still failing, is a test in to-importFn.test.ts. @tmeasday, can you tell if they are a legitimate failure, or just an artifact of how the test is being set up and run?

Oh, and I don't have a windows box set up to run tests on, so if you do @tmeasday, and wouldn't mind running them, that would be much appreciated. I think there's also a way to set up github actions to run on a windows vm... I'll submit an experiment in a separate PR to see if that might be feasible. (Here it is, for whatever it's worth: #16744)

[shilman]

Unit tests are still failing?

toImportFn - webpackIncludeRegexp › matches only suitable paths - ../src/**/*.stories.tsx

    - Expected  - 1
    + Received  + 5

    - Array []
    + Array [
    +   "/Users/user/code/.storybook/src/Icon.stories.tsx",
    +   "/Users/user/code/.storybook/src/components/Icon.stories.tsx",
    +   "/Users/user/code/.storybook/src/components/Icon.stories/Icon.stories.tsx",
    + ]

[IanVS]

Yes, it's unclear to me if those are legitimate failures (if so it I believe it would be an existing issue prior to this PR), or if it is due to the way the test itself is constructed. I'm hoping @tmeasday has some insight.

[tmeasday]

@IanVS I'm not sure exactly what's going on with these tests, but what's weird about this code is the toImportFn only matches the RHS of the path (that's because webpack passes the full absolute path into the regex, but we don't want to build absolute paths into the user's build).

So '../src/**/*.stories.tsx', should match '/Users/user/code/src/Icon.stories.tsx', because in the original test the configDir was '/path', so you'd end up with '/src/**/*.stories.tsx', which does match the RHS of that filename.

Kind of a confusing/weird test though.

After your changes, the configDir is the much more sane '/Users/user/code/.storybook' , so after resolution the directory would be '/Users/user/code/src/**/stories.tsx', which I still would have thought matches?

I would have thought it was the next test '../../src/**/*.stories.tsx' that should have broken 🤔

[tmeasday]

After your changes, the configDir is the much more sane '/Users/user/code/.storybook' , so after resolution the directory would be '/Users/user/code/src/**/stories.tsx', which I still would have thought matches?

OK, retract that, the resolution is just relative to the workingDir, so in this case it would resolve to './src/**/stories.tsx'

[tmeasday]

Ahh, sorry I finally get it, sorry was a bit slow there. So yeah, the reason, is as I said:

what's weird about this code is the toImportFn only matches the RHS of the path (that's because webpack passes the full absolute path into the regex, but we don't want to build absolute paths into the user's build).

So a matcher like './src/**/stories.tsx' will end up matching ./.storybook/src/x/y/z.stories.tsx, because we only check the RHS and don't build any paths into our match that aren't already in the SB (i.e. we don't replace '.' with workingDir).

Does that make sense @IanVS ?

[IanVS]

My understanding must be flawed, but I would have thought that if the config dir is /Users/user/code/.storybook, and the pattern is relative to that directory, then ../src/** wouldn't include anything inside /Users/user/code/.storybook, since src and .storybook are sister directories. The glob is going back up to the parent, and then down another path, right? I'm on discord for the next few minutes if it would be easier to chat there. That said, if you think these are valid paths, I'm happy to either move them into the valid section, or delete them altogether since they're kind of wonky. ;)

[IanVS]

I talked with Tom, he's going to comment out those tests and leave a note on why they are not really possible to test (due to webpack limitations) just in case anyone wonders about it in the future.