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 full page rendering and restrict rendering into head #585

Closed
wants to merge 2 commits into from
Closed

Remove full page rendering and restrict rendering into head #585

wants to merge 2 commits into from

Conversation

andreypopp
Copy link
Contributor

As per #515.

@andreypopp
Copy link
Contributor Author

Test failures seem to be unrelated to the PR.

@jordwalke
Copy link
Contributor

Before we do this, let's settle on a good router that we can use in react-page and encourage others to use the same. @andreypopp: Maybe we can settle on something like https://github.com/andreypopp/react-app-controller or a backbone router, and land it into contrib first. I don't have any strong opinions about the details of the router. I'm sure that if we found it lacking features, you'd be glad to implement missing features, correct? There's a couple of small blockers completely unrelated to the actual details of the router.

  • react-app-controller uses npm's react-tools. react-page has figured out a way to allow us to require('React') - not require('react-tools').React. The solution there, is to make a sub-project package.json file, possibly in the React core code-base and simply point your package.json at that. We should not be writing require('react-tools').React in any code base. All this because we don't have the npm name registered for React :/ but if we ever do acquire that npm name, we can just switch the package.json and touch any code.

@andreypopp
Copy link
Contributor Author

@andreypopp: Maybe we can settle on something like https://github.com/andreypopp/react-app-controller or a backbone router, and land it into contrib first. I don't have any strong opinions about the details of the router. I'm sure that if we found it lacking features, you'd be glad to implement missing features, correct?

Correct.

react-app-controller uses npm's react-tools. react-page has figured out a way to allow us to require('React') - not require('react-tools').React. The solution there, is to make a sub-project package.json file, possibly in the React core code-base and simply point your package.json at that. We should not be writing require('react-tools').React in any code base. All this because we don't have the npm name registered for React :/ but if we ever do acquire that npm name, we can just switch the package.json and touch any code.

Do you think #627 solves this? It would allow var React = require('react-core').

I don't think allowing to do require('React') when we don't own the package on npm is a good idea — that introduces another layer of indirection and I'd rather just explain all the packaging as "it's just npm".

@jordwalke
Copy link
Contributor

Regrading require('react-core'):

There's many reasons why we want to use require('react'), but one really important one is that it will make it easy for us to open source any awesome UI components that Facebook builds internally to the world. Since internally, we use require('React').

I'd rather just explain all the packaging as "it's just npm".

We can sill say: "It's just npm" - because npm allows pointing to arbitrary git repos/versions. npm has this strange tacked on feature of having a name registry as well - which is a questionable choice IMHO, but I'm grateful that it doesn't lock us into that name registry. But (in parallel) we're working on getting the proper setup on npm's name registry as well. Stay tuned and cross your fingers.

@petehunt
Copy link
Contributor

petehunt commented Dec 4, 2013

Feel free to depend on http://petehunt.net/react/react-0.5.1.tar.gz and require('react'). The advantage of this package is that if you build with NODE_ENV='production' you'll get runtime checks stripped.

@andreypopp
Copy link
Contributor Author

Feel free to depend on http://petehunt.net/react/react-0.5.1.tar.gz and require('react'). The advantage of this package is that if you build with NODE_ENV='production' you'll get runtime checks stripped.

done

@ghost ghost assigned petehunt Dec 31, 2013
@petehunt
Copy link
Contributor

petehunt commented Jan 6, 2014

Made some changes which mitigate these problems: https://groups.google.com/forum/#!topic/reactjs/4jI5xe7TXzQ

As such I think we'll keep this capability around, at least for now.

@petehunt petehunt closed this Jan 6, 2014
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.

None yet

3 participants