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

Drop React from peerDependency and rely on passing it in. #24

Merged
merged 1 commit into from May 13, 2015
Merged

Drop React from peerDependency and rely on passing it in. #24

merged 1 commit into from May 13, 2015

Conversation

AsaAyers
Copy link
Contributor

With this PR instead of react-a11y requiring React directly, you just pass your own React in.

var React = require('react');
var a11y = require('react-a11y');
if (ENV === 'development') a11y(React)

Closes #23
Closes #18
Fixes #16

Note: This and all of my other PRs are available with npm install @asaayers/react-a11y
@asaayers/react-a11y

@mathieumg
Copy link

I want to see this merged as well.

@AsaAyers Why did you change the name of the package in your fork? Prevents me from using it. In such cases I usually just change the dependency origin to the one of the fork I want, rather than installing a package with a different (and possibly invalid) name.

@AsaAyers
Copy link
Contributor Author

AsaAyers commented May 5, 2015

You have to change the name when it's a namespaced package on npm. I wouldn't have changed it if I could have voided it.

How does it prevent you from using it? Seems to me like you should have require('react-a11y') in one place in your code and it has to change to require('@asaayers/react-a11y').

@mathieumg
Copy link

You can npm install things directly from git commits/hashes/tags and this is what I do for "temporary forks" such as this. I want to change what the dependency points to, not the dependency's name.

@AsaAyers
Copy link
Contributor Author

AsaAyers commented May 6, 2015

It usually doesn't if the project is using a transpiles. I'd bet you can't
install this project from git
On May 5, 2015 9:27 PM, "Mathieu M-Gosselin" [email protected]
wrote:

You can npm install things directly from git commits/hashes/tags this is
what I do for "temporary forks" such as this. I want to change what the
dependency points to, not the dependency's name.


Reply to this email directly or view it on GitHub
#24 (comment).

@AsaAyers
Copy link
Contributor Author

AsaAyers commented May 6, 2015

I just checked, this project can't be installed from git.

asa@asa /tmp/foo % npm install git://github.com/rackt/react-a11y.git
npm WARN peerDependencies The peer dependency [email protected] included from react-a11y will no
npm WARN peerDependencies longer be automatically installed to fulfill the peerDependency 
npm WARN peerDependencies in npm 3+. Your application will need to depend on it explicitly.
[email protected] node_modules/react
└── [email protected] ([email protected], [email protected])

[email protected] node_modules/react-a11y
asa@asa /tmp/foo % node
> require('react-a11y')
Error: Cannot find module 'react-a11y'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at repl:1:2
    at REPLServer.self.eval (repl.js:110:21)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.emit (events.js:95:17)
    at Interface._onLine (readline.js:203:10)
    at Interface._line (readline.js:532:8)

The reason is the package.json points to a js file that has to be compiled for the project to work:

asa@asa /tmp/foo % grep '"main":' node_modules/react-a11y/package.json
  "main": "./dist/index.js",
asa@asa /tmp/foo % ls node_modules/react-a11y/dist
ls: cannot access node_modules/react-a11y/dist: No such file or directory

@AsaAyers
Copy link
Contributor Author

AsaAyers commented May 6, 2015

I'd love for my fork to be temporary but I don't think it will be. I'm pretty sure there's some conflicts, but when I get a chance I'll probably pull #26 into my fork too.

@kloots
Copy link
Collaborator

kloots commented May 12, 2015

Assigning to Angus to evaluate.

@AsaAyers
Copy link
Contributor Author

asa@asa /tmp/foo % npm install react-a11y
npm WARN package.json [email protected] No repository field.
npm WARN peerDependencies The peer dependency [email protected] included from react-a11y will no
npm WARN peerDependencies longer be automatically installed to fulfill the peerDependency 
npm WARN peerDependencies in npm 3+. Your application will need to depend on it explicitly.
[email protected] node_modules/react
└── [email protected] ([email protected], [email protected])

@kloots kloots assigned kloots and unassigned angus-c May 13, 2015
kloots added a commit that referenced this pull request May 13, 2015
Drop React from peerDependency and rely on passing it in.
@kloots kloots merged commit 34533d7 into reactjs:master May 13, 2015
@mathieumg
Copy link

Awesome! Sorry for the delay @AsaAyers I've been caught up in other things, but it seems like it's no longer relevant.

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.

React 0.13 support (conflicting peerDependency)
4 participants