[Bug?]: Bug in mock-auth plugin's RegEx breaks storybook-vite

Philzen commented 1 month ago

What's not working?

Having recently upgraded our stack to RW 7.7.4 and also migrated storybook to Vite, i today found that it suddenly stopped rendering any stories whatsoever, throwing:

3:49:51 PM [vite] Internal server error: Transform failed with 1 error:
…/web/src/auth.ts:2:9: ERROR: Expected identifier but found ","
  Plugin: vite:esbuild
  File: …/web/src/auth.ts:3:6

  Expected identifier but found ","
  1  |  import { createAuthentication as createAuth } from '@redwoodjs/testing/dist/web/mockAuth.js'
  2  |  import { , createDbAuthClient } from '@redwoodjs/auth-dbauth-web';
     |           ^
  3  |  const dbAuthClient = createDbAuthClient();
  4  |  export const {

After some headscratching about the sudden breakage i realized we had a change to auth.ts import order – before it was:

import { createDbAuthClient, createAuth } from '@redwoodjs/auth-dbauth-web'

which worked fine, but now (b/c we added an eslint rule to sort them alphabetically to get nicer, deterministic import diffs) it is

import { createAuth, createDbAuthClient } from '@redwoodjs/auth-dbauth-web'

and breaks when starting storybook-vite.

After some digging, i assume the culprit is this RegEx:

https://github.com/redwoodjs/redwood/blob/62509c9e8ca8eb6553addeadfa58126c470fb6d1/packages/storybook/src/plugins/mock-auth.ts#L14

How do we reproduce the bug?

Apparently it happens when createAuth is followed by a comma, as in a list of named imports.

What's your environment? (If it applies)

System:
    OS: Linux 6.6 Manjaro Linux
    Shell: 5.2.32 - /bin/bash
  Binaries:
    Node: 18.20.2 - /tmp/xfs-5e82201c/node
    Yarn: 4.3.0 - /tmp/xfs-5e82201c/yarn
  Databases:
    SQLite: 3.46.1 - /usr/bin/sqlite3
  npmPackages:
    @redwoodjs/auth-dbauth-setup: 7.7.4 => 7.7.4 
    @redwoodjs/cli-storybook-vite: 7.7.4 => 7.7.4 
    @redwoodjs/core: 7.7.4 => 7.7.4 
    @redwoodjs/project-config: 7.7.4 => 7.7.4 
  redwood.toml:
    [web]
      title = "Interviewtool"
      port = 8910
      apiUrl = "/.redwood/functions" # you can customise graphql and dbauth urls individually too: see https://redwoodjs.com/docs/app-configuration-redwood-toml#api-paths
      includeEnvironmentVariables = [] # any ENV vars that should be available to the web side, see https://redwoodjs.com/docs/environment-variables#web
      # host = "0.0.0.0"  # listen to all interfaces – for local network testing purposes only
    [api]
      port = 8911
      debugPort = 18911 # https://redwoodjs.com/docs/project-configuration-dev-test-build#customizing-the-debug-port
      # host = "0.0.0.0"  # listen to all interfaces – for local network testing purposes only
    [browser]
      open = false

Are you interested in working on this?

[x] I'm interested in working on this

ahaywood commented 1 month ago

@Philzen Thanks for taking the time to report! ... and I see you have a PR in! 🙌 I know @arimendelow has been working on Storybook updates here #11581 I'll get him to review your stuff.

arimendelow commented 1 month ago

Ha, this is interesting — @Philzen, just reviewed your other PR and left a comment on it.

A short term fix here, of course, is to just ignore ESLint on this line, such that your imports aren't reordered.

@ahaywood, do you think it's worth adding a note in #11581 to be careful when reordering any Redwood-generated imports? I feel like trying to include something about this in the docs would be confusing without the proper context.

@Philzen, if you want to open a PR for fixing this, I'd be happy to review it! As you can see from your own excellent investigation, all that needs to happen here is removing createAuth from the original import so that we can replace it with the mocked version.

Philzen commented 1 month ago

@ahaywood pls note the other PR was completely unrelated to this issue.

do you think it's worth adding a note in https://github.com/redwoodjs/redwood/pull/11581 to be careful when reordering any Redwood-generated imports?

@arimendelow i found https://github.com/redwoodjs/redwood/blob/main/packages/eslint-config/README.md very helpful for this (which is to from the docs) (my two cents would be that a json config with "$schema": "https://json.schemastore.org/eslintrc.json" gives much superior documentation and code completion than the js file – even when annotated with a JSDoc type import).

Everyone having ever tried to extend an already extensive ruleset such as @redwoodjs/eslint-config gets the drift pretty quickly that there be dragons :D And i understand that a least since the decoupling of auth.ts from the auth-packages (i remember that happening somewhat around v4.x) these are free for customizations, so any kind of additional imports may appear there all the time.

So therefore IMHO the regex is simply making an assumption here that it shouldn't rely on. I'm no RegEx guru, but played around with it a bit and believe the change proposed in #11593 seems to do the trick, kindly take a gander.

arimendelow commented 1 month ago

Left a comment on your PR! Looks mostly good to me :) Thanks for putting that together so quickly!

These are all great points 😄 I'm no RegEx guru either, hence us winding up here in the first place — your proposal is certainly an improvement.

redwoodjs / redwood