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

Generated comments are redundant and easily conflicted #10

Closed
xareelee opened this issue Mar 20, 2017 · 11 comments
Closed

Generated comments are redundant and easily conflicted #10

xareelee opened this issue Mar 20, 2017 · 11 comments

Comments

@xareelee
Copy link

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
Copy link
Owner

elderfo commented Mar 20, 2017

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

@elderfo
Copy link
Owner

elderfo commented Mar 20, 2017

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

xareelee commented Mar 20, 2017

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
Copy link
Owner

elderfo commented Mar 20, 2017

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

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

@elderfo
Copy link
Owner

elderfo commented Mar 20, 2017

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

@xareelee
Copy link
Author

xareelee commented Mar 20, 2017

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
Copy link
Owner

elderfo commented Mar 20, 2017

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
Copy link
Owner

elderfo commented Mar 20, 2017

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

@elderfo

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

@elderfo
Copy link
Owner

elderfo commented Mar 20, 2017

Fixed in v1.1.0

michaelbayday pushed a commit to michaelbayday/react-native-storybook-loader that referenced this issue Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants