search

wix-incubator / react-module-container

Small library for building micro apps in React and Angular
MIT License
69 stars 18 forks source link

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

Open deanshub opened 5 years ago

ronenst commented 5 years ago

@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 commented 5 years ago

@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 (https://github.com/facebook/react/issues/10320#issuecomment-318754280) 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 commented 5 years ago

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 commented 5 years ago

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

ronenst commented 5 years ago

Okay, thanks, we'll discuss internally.