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

moving react and react-dom to dev-dep #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deanshub
Copy link

@deanshub deanshub commented Aug 6, 2019

No description provided.

@deanshub deanshub requested a review from yanivefraim August 6, 2019 10:30
@ronenst
Copy link
Contributor

ronenst commented Aug 6, 2019

@deanshub Can you explain the reason for this change?

I don't think it's correct and we've had problems with code like this before. An angular project for example should be able to import and define an AngularLazyComponent without having to explicitly import react.

We do need to update the version though...

@deanshub
Copy link
Author

deanshub commented Aug 6, 2019

@ronenst that's exactly my point, I moved react and react-dom from dependencies to dev-dependencies.
I have an issue that a project of mine that I'm upgrading to react 16, uses a dependency that uses this project, when I'm installing dependencies it installs react 15 because it's a regular dependency and not dev which causes an error of conflicting react versions (facebook/react#10320 (comment))
once moving these dependencies to dev-dependency they won't be installed and the react will come from the parent's dependency which makes more since since this is a library and not a project

@ronenst
Copy link
Contributor

ronenst commented Aug 6, 2019

Do you have react and react-dom defined as externals? That may solve it but you'll need to inject them via script tags.

What worries me is that this change could potentially break projects which don't install React themselves. Even if it is more correct that could be a problem.

Our team can try upgrading the version though, I think that should also solve your issue?

@deanshub
Copy link
Author

deanshub commented Aug 6, 2019

I have a workaround, just thought it would be better to move it here as-well

@ronenst
Copy link
Contributor

ronenst commented Aug 6, 2019

Okay, thanks, we'll discuss internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants