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

Remove the reactify transform from package.json #11

Closed
mvila opened this issue Jun 15, 2015 · 6 comments
Closed

Remove the reactify transform from package.json #11

mvila opened this issue Jun 15, 2015 · 6 comments

Comments

@mvila
Copy link

mvila commented Jun 15, 2015

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
Copy link
Owner

joshwnj commented Jun 17, 2015

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
Copy link
Author

mvila commented Jun 19, 2015

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
Copy link
Owner

joshwnj commented Jun 22, 2015

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
Copy link
Author

mvila commented Jun 28, 2015

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
Copy link
Owner

joshwnj commented Oct 26, 2015

@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
Copy link
Owner

joshwnj commented Oct 27, 2015

This is resolved in v3.0.0

@joshwnj joshwnj closed this as completed Oct 27, 2015
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

No branches or pull requests

2 participants