Skip to content

Conversation

@jtmalinowski
Copy link
Contributor

Hi guys,
Great job! React is awesome.

A little PR incoming...

I'm not sure if I correctly guessed that

if (typeof window === "undefined" || window === null) {
  return;
}

is there to allow JSXTransformer.js to run outside of browser environment. If so, then it's necessary to move reference to document to be after this if. Also it might be nice to have a regression for this.

Otherwise, any hints on how to prepare non-browser non-node build of JSXTransformer? I'm working on a gem to simply compile jsxes server side in rails (sprockets engine). If you think it suits this repo then I can PR it too.

@zpao
Copy link
Member

zpao commented Jul 20, 2013

Nice! We'll definitely take this. I'm on my phone and can't check right now, but if you haven't already, can you sign the CLA (https://developers.facebook.com/opensource/cla)?

Also, I'm excited to see somebody working on (what I would call react-rails)! I started to after initial launch but got sidetracked. I actually set up http://rubygems.org/gems/react-source to get started on this (following the same pattern as coffeescript and ember I think - having 2 distinct gems). I got hung up on the non-node thing too but never figured out the best way. We're going to have slight discrepancies between bin/jsx and this so we should figure out the best thing to do about that.

If we do react-rails in this repo, we'll need to shuffle some things around. Lets chat and figure out the best way to do this

@jtmalinowski
Copy link
Contributor Author

Yeah, I signed the CLA.

I would go for how it's done in ember, so react-source could be left as it is, and we could nearly just copy ember-rails and rename a few things.

I'm not sure what you mean by discrepancies, you probably meant main.js instead of bin/jsx right? Because bin/jsx does not export anything. And you're right here, load and run should not be exported when window/document is not available. If I understand it right here, I'll fix it. Anything beside this?

In case of trying to fit react-rails into here, we would need to the whole vendor/, I'm not sure if it makes sense. Any reason not to go the Ember way?

@zpao
Copy link
Member

zpao commented Jul 20, 2013

By discrepancies I mean that the transform is slightly different. bin/jsx gives a little bit more power and options (like stripping __DEV__, transforming into common js). I think it's fine for them to differ a bit though, so long as they have the same default behavior.

As for getting react-rails in here, I think we'd need to restructure the repo a little bit (ie lib/ is set up for react-source, but we'd want a different lib/ for react-rails, so I'm thinking we would put each under it's own directory, or both under a gems/ dir). Otherwise, I think the ember-rails route makes sense.

load and run should not be exported when window/document is not available. If I understand it right here, I'll fix it. Anything beside this?

I think that should do it! Do that and I'll merge this in.

@jtmalinowski
Copy link
Contributor Author

It seems like adding an acceptance test for this would be an overkill?

@zpao
Copy link
Member

zpao commented Jul 23, 2013

Yea, no need for tests here. Thanks!

zpao added a commit that referenced this pull request Jul 24, 2013
Allow to execute JSXTransformer outside of browser environment
@zpao zpao merged commit 8dd4428 into facebook:master Jul 24, 2013
@jtmalinowski jtmalinowski deleted the non-browser-env branch July 24, 2013 11:16
@jtmalinowski jtmalinowski mentioned this pull request Jul 24, 2013
@zpao zpao mentioned this pull request Jul 24, 2013
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.

2 participants