elderfo / react-native-storybook-loader

An automatic story loader for react-native-storybooks
https://github.com/elderfo/react-native-storybook-loader
MIT License
299 stars 27 forks source link

Generated comments are redundant and easily conflicted #10

Closed xareelee closed 7 years ago

xareelee commented 7 years ago

This is my storyLoader.js which was generated by this library:


// template for doT (https://github.com/olado/doT)

function loadStories() {

  // the username and project path would be different on different machines
  // which lead to the commit conflict.
  require('./../src/ui/CountryPolicy/CountryPolicy.stories.js'); // /Users/{username}/{project_path}/src/ui/CountryPolicy/CountryPolicy.stories.js

  // ...  other paths
}

module.exports = {
  loadStories,
};

The problem is the generated comments.

When different developers run the loader ./node_modules/.bin/rnstl to update the storyLoader.js file, the generated comments will always conflict with different user's update due to the comments containing the absolute paths with the username and the project path.

I think that removing the redundant comments is better.

elderfo commented 7 years ago

That makes sense. If you want to submit a PR, that would be awesome. Otherwise, I can get to it later this week.

elderfo commented 7 years ago

Honestly, I would have preferred abstract away the story loader. So your index.android.js and index.ios.js files would be more like this:

import { AppRegistry } from 'react-native';
import { getStorybookUI, configure } from '@kadira/react-native-storybook';
import { loadStories } from 'react-native-storybook-loader';

// import stories
configure(loadStories, module);

const StorybookUI = getStorybookUI({port: 7007, host: 'localhost'});
AppRegistry.registerComponent('ReactNative', () => StorybookUI);

I would personally prefer to keep generated files out of source control. This discussion is probably outside the scope of this ticket, but I wanted to present it to you.

xareelee commented 7 years ago

Thanks for your reply.

This does not work for me:

import { loadStories } from 'react-native-storybook-loader';

It shows Unable to resolve module 'react-native-storybook-loader' on the simulator.

I use Jest as the test runner. I think it is better to run the loader without generating any output file each time before Jest starts . Jest provides setupFiles for this purpose.

elderfo commented 7 years ago

I know that would cause issues with watcher's too, since the generated file would live under node_modules and I believe most watchers ignore that.

How are you using setupFiles, are you wrapping the loader into a script called by that? I haven't had a reason to use that config option yet.

xareelee commented 7 years ago

By the way, could you make the loading sequence in alphabetical order?

elderfo commented 7 years ago

Totally, I will create another issue for the sorting. #11

xareelee commented 7 years ago

Maybe I should call react-native-storybook-loader each time before running Jest/Storybook, and ignore the generated storyLoader.js in the git repo to avoid any unnecessary conflict.

I don't think storyLoader.js should be tracked.

elderfo commented 7 years ago

I agree that storyLoader.js should not be tracked. On the projects i've used it on, i exclude the storyLoader.js from linting and the git repo.

This is why I want to implement loading stories from the module. My only concern is that there will be issues with webpack or other utilities that perform watches. I guess I will have to try it out. I will open another issue for that.

elderfo commented 7 years ago

Since that may be a larger undertaking, I will fix this issue and the sorting one and kick out a new version for you.

xareelee commented 7 years ago

@elderfo

Thanks for your help and contribution! I'm looking forward to it. 💃

elderfo commented 7 years ago

Fixed in v1.1.0