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

Fix JSXTransformer when therubyracer is used by execjs #16

Merged
merged 2 commits into from
Dec 2, 2013

Conversation

quark-zju
Copy link
Contributor

The execjs gem will try to use native v8 js library therubyracer if it is available. Otherwise execjs will try to execute external node, which is the case in current tests.

The problem is, node has global defined but therubyracer doesn't:

require 'v8'
require 'execjs'

context = ExecJS.compile('') 
puts context.class # ExecJS::RubyRacerRuntime::Context
puts context.eval('global')  # ReferenceError: global is not defined (ExecJS::ProgramError)

JSX cannot work properly with neither global nor window defined. This pull request fixes it by defining global. I also added tests for the change and fix some other small issues I found.

For adding gemfiles directory to .gitignore, I know the community may suggest include Gemfile and Gemfile.lock in source control. But here is different. Reasons:

  • gemfiles are generated by appraisals, not manually created. files generated by auto tasks should live outside source control
  • .lock files contain developer-dependent path, "/Users/poshannessy/FB/code/react-rails"
  • other project benifits from .lock file is because they can use same gem version as the developer, to avoid potencial problems. however, gemfile{,.lock}s are only used for testing purpose here, i think it's a good idea to not lock any versions but always install the latest. if another gem update acturally breaks this project, know earlier.

That being said, the Gemfile and Gemfile.lock at the root directory should not be removed.

@jtmalinowski
Copy link
Collaborator

We had a longer discussion on IRC about gemfiles and decided to have them in the repo. AFAIK argument no. 2, i.e. developer-specific paths, is either already outdated or will soon be, because (thoughtbot/appraisal#49) bundler 1.4 makes these paths relative. I haven't checked yet though.

Anyway, such changes as including gemfiles or not should belong to a separate PR I think.

I would ask you to be patient for a few next days, because I will be finishing #15 and #12 even though they are (especially #12) seemingly dead. I hope we'll then have your PR rebased against master and we'll rethink if it's an optimal solution.

@zpao
Copy link
Member

zpao commented Nov 28, 2013

First, thanks for catching this (and adding a test)! I'll have a few comments inline.

But first... Ok, I thought about the .lock files again. I know we previously talked about keeping them but that was when the Appraisal authors suggested it. It looks like they've gone back on that, so I'll remove them (and update the .gitignore appropriately). But that's not the important thing here. But as for keeping gemfiles, I'm going to disagree there and keep them - they just contain what's in the Appraisals file, so there's only explicit version locking.

@zpao
Copy link
Member

zpao commented Nov 28, 2013

Also, I don't want to block anybody if it can be avoided, so whichever doesn't get it first will need to rebase and test appropriately.

@@ -6,12 +6,14 @@ module React
module JSX
def self.context
# TODO: create React::Source::contents_for
contents = File.read(React::Source.bundled_path_for('JSXTransformer.js'))
contents =
'if("undefined"==typeof global)global={};' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's cleanup the JS a bit and then reformat while we're here. We'll take advantage of the fact that we're in a clean context so this is the global which is guaranteed to exist.

contents =
  'var global = global || this;' +
  File.read(React::Source.bundled_path_for('JSXTransformer.js'))

Can you also add a comment above saying why we're doing this (JSXTransformer has UMD wrapper which only checks for window, global, and self but a clean context in therubyracer doesn't have any of those)

@quark-zju
Copy link
Contributor Author

Thanks for your comments! I have addressed them. Just curious, is there a planned release after merging #15, #12 and this one?

@zpao
Copy link
Member

zpao commented Dec 2, 2013

Thanks a lot for the quick followup!

I actually think I'll push out a release today with just this. #15 is already in. #12 is going to include a major version bump and start diverging from the React version number scheme we have now. Or at least it was from what I remember, though that change isn't in there now.

zpao added a commit that referenced this pull request Dec 2, 2013
Fix JSXTransformer when therubyracer is used by execjs
@zpao zpao merged commit c1081ab into reactjs:master Dec 2, 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.

3 participants