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?]: Bug in mock-auth plugin's RegEx breaks storybook-vite #11588

Closed
1 task done
Philzen opened this issue Sep 19, 2024 · 4 comments · Fixed by #11593
Closed
1 task done

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

Philzen opened this issue Sep 19, 2024 · 4 comments · Fixed by #11593
Labels
bug/needs-info More information is needed for reproduction

Comments

@Philzen
Copy link
Contributor

Philzen commented Sep 19, 2024

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:

/(import\s*{\s*[^}]*)(\bcreateAuth\b)([^}]*})/,

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?

  • I'm interested in working on this
@Philzen Philzen added the bug/needs-info More information is needed for reproduction label Sep 19, 2024
@Philzen Philzen changed the title [Bug?]: Bug mock-auth.ts RegEx breaks storybook (vite) [Bug?]: Bug mock-auth.ts RegEx breaks storybook-vite Sep 19, 2024
@Philzen Philzen changed the title [Bug?]: Bug mock-auth.ts RegEx breaks storybook-vite [Bug?]: Bug in mock-auth plugin's RegEx breaks storybook-vite Sep 19, 2024
@ahaywood
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

Philzen commented Sep 19, 2024

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

do you think it's worth adding a note in #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
Copy link
Contributor

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.

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.

3 participants