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

Fix Storybook config detection by adding serverRequire to interpret files #747

Merged

Conversation

JonathanKolnik
Copy link
Contributor

There are cases where

const r = typeof __non_webpack_require__ !== 'undefined' ? __non_webpack_require__ : require;

still is undefined

so now we fallback to the way storybook interprets these modules

❓ should we just always do the storybook pattern?

@linear
Copy link

linear bot commented May 10, 2023

AP-2756 Chromatic telemetry reports `null` `storybook_version`

How is the user affected?

Our telemetry reports storybook_version = null for a large proportion of our build events:

image.png

How many and/or what class of users does this impact?

Unsure, possible all users, or maybe a specific CLI version / SB version

Is there a workaround?

Backfill data based on other data.

What are the steps for reproducing the issue?

Not sure.

Other information

Run this query to see the above data (run on Jan 18):

SELECT count(*) as count, storybook_version FROM `warehouse-production-209019.chromatic_webapp2.create_ci_build_view`
WHERE extract(year from timestamp) = 2023
GROUP BY storybook_version
ORDER BY count DESC


// if (registered === false && !hasEsbuildBeenRegistered) {
// // eslint-disable-next-line global-require
// const { register } = require('esbuild-register/dist/node');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing the compilation error:

ERROR in ./bin-src/lib/interpret-require.ts 11:29-66
Module not found: Error: Can't resolve 'esbuild-register/dist/node' in '/Users/jonathankolnik/chromatic-cli/bin-src/lib'
resolve 'esbuild-register/dist/node' in '/Users/jonathankolnik/chromatic-cli/bin-src/lib'
  Parsed request is a module
  using description file: /Users/jonathankolnik/chromatic-cli/package.json (relative path: ./bin-src/lib)
    resolve as module
      /Users/jonathankolnik/chromatic-cli/bin-src/lib/node_modules doesn't exist or is not a directory
      /Users/jonathankolnik/chromatic-cli/bin-src/node_modules doesn't exist or is not a directory
      looking for modules in /Users/jonathankolnik/chromatic-cli/node_modules
        /Users/jonathankolnik/chromatic-cli/node_modules/esbuild-register doesn't exist
      looking for modules in /Users/jonathankolnik/node_modules
        /Users/jonathankolnik/node_modules/esbuild-register doesn't exist
      /Users/node_modules doesn't exist or is not a directory
      /node_modules doesn't exist or is not a directory
 @ ./bin-src/lib/getStorybookMetadata.ts 26:28-58
 @ ./bin-src/lib/getStorybookInfo.ts 18:31-64
 @ ./bin-src/tasks/storybookInfo.ts 16:43-77
 @ ./bin-src/tasks/index.ts 15:40-66
 @ ./bin-src/main.ts 31:32-50
 @ ./bin-src/register.js 11:14-36

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably since esbuild is not a CLI dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bundle:bin   ] ERROR in ./node_modules/esbuild/lib/main.d.ts 1:7
[bundle:bin   ] Module parse failed: Unexpected token (1:7)
[bundle:bin   ] You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
[bundle:bin   ] > export type Platform = 'browser' | 'node' | 'neutral'
[bundle:bin   ] | export type Format = 'iife' | 'cjs' | 'esm'
[bundle:bin   ] | export type Loader = 'base64' | 'binary' | 'copy' | 'css' | 'dataurl' | 'default' | 'empty' | 'file' | 'js' | 'json' | 'jsx' | 'text' | 'ts' | 'tsx'
[bundle:bin   ]  @ ./node_modules/esbuild/lib/ sync ^.*\/.*$ ./main.d.ts ./main.d
[bundle:bin   ]  @ ./node_modules/esbuild/lib/main.js 1826:14-50
[bundle:bin   ]  @ ./node_modules/esbuild-register/dist/node.js 4622:15-33
[bundle:bin   ]  @ ./bin-src/lib/interpret-require.ts 11:29-66
[bundle:bin   ]  @ ./bin-src/lib/getStorybookMetadata.ts 26:28-58
[bundle:bin   ]  @ ./bin-src/lib/getStorybookInfo.ts 18:31-64
[bundle:bin   ]  @ ./bin-src/tasks/storybookInfo.ts 16:43-77
[bundle:bin   ]  @ ./bin-src/tasks/index.ts 15:40-66
[bundle:bin   ]  @ ./bin-src/main.ts 31:32-50
[bundle:bin   ]  @ ./bin-src/register.js 11:14-36
[bundle:bin   ]
[bundle:bin   ] webpack 5.76.0 compiled with 1 error and 16 warnings in 14437 ms

This is on the bundle:bin hook after installing esbuild and esbuild-register

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonathanKolnik Let's pair when you have time?

@JonathanKolnik
Copy link
Contributor Author

Apologies for the delay @tmeasday, please test 6.17.5-canary.0

@JonathanKolnik
Copy link
Contributor Author

@thafryer @tmeasday bumping this!

@tmeasday
Copy link
Member

Looks good in my test with a random SB7 project @JonathanKolnik!

image

Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Works for me!
Screenshot 2023-05-25 at 3 30 22 PM

@JonathanKolnik JonathanKolnik merged commit f05aa7b into main May 30, 2023
24 checks passed
@JonathanKolnik JonathanKolnik deleted the jono/ap-2756-chromatic-telemetry-reports-null-pt-2 branch May 30, 2023 13:23
@ghengeveld ghengeveld changed the title Chromatic telemetry was reporting null - Add serverRequire to interpret files Fix Storybook config detection by adding serverRequire to interpret files May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants