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

Continue refactoring out common code from app layers #2636

Closed
3 of 5 tasks
tmeasday opened this issue Jan 3, 2018 · 11 comments
Closed
3 of 5 tasks

Continue refactoring out common code from app layers #2636

tmeasday opened this issue Jan 3, 2018 · 11 comments
Labels
cleanup Minor cleanup style change that won't show up in release changelog maintenance User-facing maintenance tasks

Comments

@tmeasday
Copy link
Member

tmeasday commented Jan 3, 2018

Following on from #2241

Checklist:

  • Update RN
  • Refactor RN more aggressively
  • Refactor manager code
  • Refactor server code
  • Update tests/docs
@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

@igor-dv you mentioned something about wanting to separate the angular part of @storybook/core -- can you go into a little more detail?

@tmeasday tmeasday added cleanup Minor cleanup style change that won't show up in release changelog maintenance User-facing maintenance tasks labels Jan 3, 2018
@alterx
Copy link
Member

alterx commented Jan 3, 2018

@tmeasday, there are several functions for generating dynamic components and metadata repeated through the angular app and the knobs addon. We should unify them so we don't repeat ourselves

@igor-dv
Copy link
Member

igor-dv commented Jan 3, 2018

👆 + after #2564 will be one more place - storyshots.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 4, 2018

@alterx / @igor-dv do knobs and storyshots not ultimately depend on @storybook/angular though? In which case the common functionality could maybe just stay in that package?

@igor-dv
Copy link
Member

igor-dv commented Jan 4, 2018

I think none of the addons currently are dependant on any of the app packages. We can technically do it (like @Hypnosphi suggested - just import things from extraneous deps, without defining them in package.json), but in this case, it feels a bit dirty to me - because of the purpose of the "app" package.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 4, 2018

I think we need to figure out the story on the way that addons import from the various app packages anyway, right? I mean it's pretty hacky in storyshots as it stands, it would be best to figure out a pattern as we take it further.

I guess what I am wondering is whether it helps for (say) @storybook/addon-storyshots to depend on @storybook/angular-core instead of depending on @storybook/angular. In both cases non-angular storyshots users are going to get angular dependencies that they don't want. So it will need to peer-depend or optional depend (or something else?) in any case.

@alterx
Copy link
Member

alterx commented Jan 4, 2018

Yeah, I think this was a point of discussion when we I initially started working on the knobs addon. At that point in time we wanted to keep their functionality separated, after all just because you use storybook it doesn't mean you'll want to use the knobs addon. I still feel the same way.

We should split out the metadata/component/module annotation stuff from both the app and the addons; these things aren't directly related to the app or addons. They're just things we need in order to make storybook work with angular as it stands.

@stale
Copy link

stale bot commented Feb 18, 2018

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!

@stale stale bot added the inactive label Feb 18, 2018
@igor-dv
Copy link
Member

igor-dv commented Feb 18, 2018

Can all this recent transition to the core be considered a refactoring? Or you have something else in mind?

@stale stale bot removed the inactive label Feb 18, 2018
@Hypnosphi Hypnosphi added the todo label Feb 18, 2018
@danielduan
Copy link
Member

Do you think splitting the RN app from the browser UI is something that could be added to the list of refactoring here?

Instead of the browser keeping track of the state, we should just run that code in RN. This would allow our app to run and ship independently.

@tmeasday
Copy link
Member Author

@danielduan probably best to keep that to a separate issue. What I mean in this issue is quite a simple refactoring out of repeated code from the various app layers. It has been happening, slowly on both the preview and config (server) front.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Minor cleanup style change that won't show up in release changelog maintenance User-facing maintenance tasks
Projects
None yet
Development

No branches or pull requests

6 participants