require.context implementation in storyshots differs to webpacks

[tmeasday]

Issue details

Storyshots calls require immediately, rather than later when req is called. This is completely inconsistent with what webpack does.

Steps to reproduce

// In storyshots, the require happens here:
const req = require.context('../src/components', true, /\.stories\.js$/)

// So if we add a decorator here, it won't apply to stories in storyshots
addDecorator((story) => <div><h1>Wrapper<h1>{story()}</div>

function loadStories() {
  // in webpack the require happens here:
  req.keys().forEach((filename) => req(filename))
}

configure(loadStories, module);

[Hypnosphi]

Wow, I thought require.context is an exclusive feature of webpack

UPD: Oh I see, it's a polyfill for it

[tmeasday]

Heh, it is, apart from this mildly hacky implementation: https://github.com/storybooks/storybook/blob/master/addons/storyshots/src/require_context.js

We should fix it and also we should probably publish it as a separate package, it wouldn't be surprising if other projects similarly wanted to replicate this functionality from a testing/node context.

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

[igor-dv]

Go away, StaleBot. I'll take it soon.

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[goodmind]

any updates?

[igor-dv]

Not yet =)

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[tmeasday]

ping

[Hypnosphi]

Hypnosphi added the todo label just now

this works better

[igor-dv]

I've tried this babel-plugin-require-context-hook (which is only a few days old 🍰) locally and it worked pretty well!

With this plugin, we will be able to remove our require_context implementation and just import the config.js as is. Users will need only to configure it in their babel configuration.
It will also solve #3406, which, IMHO, is not releted to us.

WDYT ?

P.S. @smrq, maybe you can share, as the owner of this package, what are your plans of maintaining it?

[tmeasday]

@igor-dv this seems like a great solution, although I am unsure about requiring the user to add it to their babel config:

a) they may not be comfortable with adding something to their production babel config just for the sake of SB

b) is it possible the plugin will break their legitimate uses of require.context in their webpack builds for their real app too?

I would say it would be better if we added the plugin to whatever babel config we detect for the user (at SB runtime) without them having to touch it. What do you think?

[smrq]

Wow, I didn't think anyone would find this thing, or at least so quickly :p I certainly plan on maintaining it for the foreseeable future, I'm using it at work for the system I'm in charge of. I don't know if there are edge cases where the polyfill doesn't exactly match the behavior of Webpack (I just implemented it based on my own understanding of the API, rather than on some defined spec), but if such a thing comes up then it shouldn't be a problem to fix at that time.

On Mon, Jun 11, 2018, 8:24 PM Tom Coleman ***@***.***> wrote: @igor-dv <https://github.com/igor-dv> this seems like a great solution, although I am unsure about requiring the user to add it to their babel config: a) they may not be comfortable with adding something to their production babel config just for the sake of SB b) is it possible the plugin will break their legitimate uses of require.context in their webpack builds for their real app too? I would say it would be better if we added the plugin to whatever babel config we detect for the user without them having to touch it. What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2894 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYd-SQOdO7msDCaW8IDkf8vIUi0pZycks5t7xhEgaJpZM4R01wp> .

-- *Greg Smith* https://github.com/smrq

[igor-dv]

@tmeasday

a)

Even though we encourage people (for web usages) to use require.context to load all the stories at once, this is still only a webpack feature and IMO babel was always a way to polyfill things like this. Also if anyone has other usages of require.context in the code, they will need to add a babel polyfill for this any way to make it work in tests.

For example, if you are using react-native-web you will probably need to add a plugin to webpack.config or babel-plugin-module-resolver to make it work. If you are using emotion you will probably wont to add babel-plugin-emotion for some of the features. I am sure there much more examples =)

b)

Users will need to add it under the "test" environment of the babel config (that is hooked on tests run only), which means that won't break the real implementation. Something like this:

{
  "presets": [
    ["env", { "modules": false }],
    "stage-0",
    "vue"
  ],
  "env": {
    "test": {
      "plugins": ["require-context-hook"]
    }
  }
}

I would say it would be better if we added the plugin to whatever babel config we detect for the user

I am not sure how to make it work. Since tests are running already after all the setups happened...

@smrq, Timing =) I was already about to do something like this by myself

[tmeasday]

Oh, I guess I hadn't quite got this round the right way. This would only be required for storyshots, right? Not for general storybook usage. If so, I think it is OK to ask the user to do something a little bit complicated like this to make it work.

(I think in general we will probably be pushing people to use require.context even more in the future, right?)

[igor-dv]

Yeah, for storyshots or tests in general =)

[Hypnosphi]

Released as 4.0.0-alpha.10

[airtonix]

Can you clarify if we need that babel plugin in this case:

• don't want to explicitly require each and every individual story file
• want to use require.context to load them.

Having been inculcated by Python, I'm usually flapping my mouth about being explicit rather than implicit. In this case though... i just want to import all stories and move on with my life.

do i need the babel plugin yay or nay? i think yay since I'm getting an error.

.storybook/config.js

import { configure } from '@storybook/react';

const stories = require.context(`${process.cwd()}/src`, true, /stor(y|ies)\.js$/);

function loadStories() {
    stories.keys().forEach((filename) => stories(filename))
}

configure(loadStories, module);

error:

__webpack_require__(...).context is not a function
    TypeError: __webpack_require__(...).context is not a function
    at loadStories (http://localhost:9001/static/iframe.bundle.js:832:66)
    at ConfigApi._renderMain (http://localhost:9001/static/iframe.bundle.js:2353:20)
    at render (http://localhost:9001/static/iframe.bundle.js:2308:17)
  

[igor-dv]

Looks like you are getting this error while just running storybook (not the from jest ), right?

[airtonix]

@igor-dv correct. I would also mention I'm using 4.0.0_alpha.24 or something (the very latest), because x0 uses webpack 4

[igor-dv]

In that case you should not configure this plugin to the main SB bulid. It should run only in context of jest

[hisapy]

Hi guys,

I've installed the babel-plugin-require-context-hook and added it to my .babelrc which is only for test but I'm still I'm having the TypeError: require.context is not a function when running the StoryShots tests.

Everything is working fine in the Storybook, just having problems from jest.

[igor-dv]

Did you also register it in the jest startup ?

[hisapy]

nope ... where do I have to do that?

[hisapy]

I just tried putting require('babel-plugin-require-context-hook/register')(); in one of my jest setup files but still getting the same error

[igor-dv]

Need some reproduction then

[hisapy]

Now I've added require('babel-plugin-require-context-hook/register')(); in my .storybook/config.js file right before using require.context and it's working in jest but my Storybook stopped working with the following error

ERROR in ./node_modules/babel-plugin-require-context-hook/register.js
Module not found: Error: Can't resolve 'fs' in '/Users/hisa/workspace/eljurista/node_modules/babel-plugin-require-context-hook'

So in conclusion, I believe that the possible solution babel-plugin-require-context-hook, mentioned in the migration guide doesn't work ... I'm rewriting my .storybook/config.js file to explicitly and manually require each one of my .story.js files

[igor-dv]

It works. In fact we are using it in the repo

[hisapy]

Well, there must be something I'm missing ... perhaps a little more guidance in the migration docs could help ... I'm going to look for where the plugin is registered in the repo to try to emulate it in my project.

[airtonix]

I'm not using jest.

…

On Wed, Oct 10, 2018, 6:54 AM Hisa ***@***.***> wrote: Well, there must be something I'm missing ... perhaps a little more guidance in the migration docs could help ... I'm going to look for where the plugin is registered in the repo to try to emulate it in my project. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2894 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADvKeslQUIJLPLqJktrnxIXecbKPZlEks5ujQXhgaJpZM4R01wp> .

[hisapy]

Based on my findings searching this repo I finally got it working on my project. Just left my .storybook/config file as it was before and added the following to my jest setup file.

import registerRequireContextHook from 'babel-plugin-require-context-hook/register';
registerRequireContextHook();

Now it works with jest, without breaking my project's Storybook
:)

[igor-dv]

@airtonix, how can we reproduce the problem?

[renemn]

@airtonix I think that's because webpack doesn't recognize variables at compile time, that could explain the message about __webpack_require__(...).context is not a function.

Have you tried by using webpack's ContextReplacementPlugin and do something like this in a custom webpack.config.js:

const path = require('path');
const webpack = require('webpack');

module.exports = (storybookBaseConfig) => {
  storybookBaseConfig.plugins.unshift(
    new webpack.ContextReplacementPlugin(
      /<cwd>/,
      path.resolve(process.cwd(), 'src')
    )
  );
  return storybookBaseConfig;
};

Then in the config file you could require the src like this:

const stories = require.context('<cwd>', true, /.stor(y|ies)\.js$/);

That could work as possible workaround since process.cwd() won't work at const stories = require.context(${process.cwd()}/src, true, /stor(y|ies)\.js$/);.

Just fyi, I'm not completely sure about the consequences of this in whole storybook's behavior tho, but it should do the job initially.