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

Make react an explicit dependency for Ember app #4289

Closed
gabrielcsapo opened this issue Oct 4, 2018 · 16 comments
Closed

Make react an explicit dependency for Ember app #4289

gabrielcsapo opened this issue Oct 4, 2018 · 16 comments

Comments

@gabrielcsapo
Copy link
Contributor

Right now we make clients who are not using react bundle react, which seems a little confusing.

@gabrielcsapo
Copy link
Contributor Author

@shilman @igor-dv continuing discussion here

@shilman shilman changed the title make react a implicit dependency of @storybook Make react an explicit dependency of @storybook for non-react apps Oct 4, 2018
@shilman shilman changed the title Make react an explicit dependency of @storybook for non-react apps Make react an explicit dependency for non-react apps Oct 4, 2018
@shilman
Copy link
Member

shilman commented Oct 4, 2018

For instance, see this PR: #4288

If we made react and react-dom a direct dep of all app/* packages, then this would be less confusing. @storybook/react and @storybook/react-native would still use peer deps to work with whatever version the user is already using.

@igor-dv @ndelangen @Hypnosphi @tmeasday Thoughts?

@tmeasday
Copy link
Member

tmeasday commented Oct 4, 2018

This seems like the right thing to do to me.

@igor-dv
Copy link
Member

igor-dv commented Oct 5, 2018

Yes. Agree.

@igor-dv
Copy link
Member

igor-dv commented Oct 5, 2018

With the same effort, let's maybe add babel-loader + @babel/core to the angular app since they don't really need babel themselves.

@gabrielcsapo
Copy link
Contributor Author

@igor-dv you think we should do this in the same cleanup?

@igor-dv
Copy link
Member

igor-dv commented Oct 6, 2018

Yeah, why not?

@Hypnosphi
Copy link
Member

@igor-dv I'm afraid we can't do that
#3335 (comment)

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 6, 2018

If we made react and react-dom a direct dep of all app/* packages

But we already have react and react-dom as a direct dep of all app/* packages except react. Both in 4.0 and 3.4

@Hypnosphi
Copy link
Member

@gabrielcsapo which particular package doesn't have an explicit dependency on react at the moment?

@igor-dv
Copy link
Member

igor-dv commented Oct 6, 2018

I'm afraid we can't do that

@Hypnosphi, are you sure it's relevant to the Angular apps as well? Everything there is TS...

@igor-dv
Copy link
Member

igor-dv commented Oct 6, 2018

which particular package doesn't have an explicit dependency on react at the moment?

looks like ember app missed it

@Hypnosphi
Copy link
Member

@igor-dv babel supports TS now, plus it can appear in node_modules because of being a dependency of some other third party package. What you suggest means relying on npm hoisting

@Hypnosphi
Copy link
Member

ember app missed it

I see. @gabrielcsapo please add those dependencies to ember app

@Hypnosphi Hypnosphi changed the title Make react an explicit dependency for non-react apps Make react an explicit dependency for Ember app Oct 6, 2018
@gabrielcsapo
Copy link
Contributor Author

Sorry for the delay just published a PR

@ndelangen
Copy link
Member

Can we close this then? PR seems to be merged.

@igor-dv igor-dv closed this as completed Oct 18, 2018
@issue-sh issue-sh bot removed the merged label Oct 18, 2018
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

6 participants