search

joshwnj / react-visibility-sensor

Sensor component for React that notifies you when it goes in or out of the window viewport.
MIT License
2.33k stars 195 forks source link

Remove the reactify transform from package.json #11

Closed mvila closed 9 years ago

mvila commented 9 years ago

Since I replaced reactify by babelify, I get the following error when I try to browserify my project using your module:

Error: Cannot find module 'reactify' from '/Users/mvila/Projects-next/avc-online/node_modules/react-visibility-sensor'

I think you should remove the reactify transform from your package.json and find another way to reactify your tests/examples.

joshwnj commented 9 years ago

Hi @mvila, thanks for the heads-up. I believe this is due to the browserify field in package.json, which instructs browserify what transforms to use: https://github.com/joshwnj/react-visibility-sensor/blob/master/package.json#L34-L40

In the most recent version I've moved reactify from devDependencies to dependencies, meaning that reactify is guaranteed to be available when needed. Could you please upgrade to the latest version of react-visibility-sensor and let me know if the problem is still occurring?

mvila commented 9 years ago

I have not tested yet but I think it should work. However, this is not great to include reactify in dependencies. Since it is only used by examples and tests, it should be in devDependencies. I think the solution is to leave reactify in devDependencies and remove the browserify field in package.json. Since you explicitly specify the reactify transform in build-exampleand build-test scripts, you don't need the browserify field.

joshwnj commented 9 years ago

Sure, I'll have a look at what some other react components are doing and see if there's a "best practice" to adopt here.

The advantage of including reactify in the package.browserify field (and I believe, the reason they recommend it) is that package authors can ensure their packages are bundled with whatever dependencies are necessary to browserify them, and package consumers don't need to have any special knowledge about what transforms they need to install.

mvila commented 9 years ago

I understand the advantage of the browserify field in package.json but it is not appropriate in your case since you use reactify only for tests and examples.

joshwnj commented 9 years ago

@mvila sorry for the delay - you make a good point and we'll be removing the package.browserify field in the next release (arriving very soon).

joshwnj commented 9 years ago

This is resolved in v3.0.0