Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Use transform directly rather than shelling out #2

Merged
merged 2 commits into from

3 participants

@petehunt

The shell command does a lot more than just desugaring; this makes the transform much, much simpler and saves you extra processes.

@ericclemmons

Thanks Pete! This looks similar to the browserify code in the library, which makes me feel much better about how I solved that :)

I'll dig into why the test is failing & update accordingly...

@petehunt

@ericclemmons So I think that Node changed the spec of readFileSync() at some point, so the local version of Node I was running is different than the one in Travis. I've updated the branch with a workaround.

@ericclemmons

I'm guessing it had something to do with readFileSync returning a buffer? It looks like you force-pushed over the previous commit, so I can't readily tell what the difference was (that sounds like something important to know) :(

@petehunt

Oh sorry! I thought you would want a single commit. I changed readFileSync(..., {encoding: 'utf8'}) to readFileSync(...).toString()

Now there is a new problem with jenkins, but it doesn't look related to this change. I think it is because react-tools expects to be installed globally? Maybe? I don't know npm that well.

@petehunt

Fixed! This was a bug in node v0.8 which prevented react-tools from installing correctly: npm/npm#2907

@ericclemmons
Owner

I did have one last question, since I've normally supported v0.8 with my plugins since that's the base version supported by gruntjs: Do you mind if I amend to your BR to make it BC compatible with ~v0.8 as well as ~v0.10?

@petehunt

Sure, but keep in mind that react-tools had issues installing itself on older versions of node. See facebook/react#132

@ericclemmons
Owner
@petehunt

To be clear, the code in React and my changes to grunt-react itself doesn't do anything specific to v0.10, it's just how react-tools specifies dependencies (so npm install fails on some earlier versions of Node). So the change to .travis.yml without changes to package.json is the right way to go, IMO.

Anything else I can do or address to get this landed? I am basing the Rendr+React integration (https://github.com/petehunt/rendr-react-template/) on this project and I think this pull is required to get it working.

Thanks for your hard work!

@ericclemmons
Owner

I was just merging this down now, but thought about how to test the ignoreMTime. I'll merge down, but would like to keep the issue open so I can create a test for it.

Let me know what the best strategy is & I'll get it done!

@ericclemmons ericclemmons merged commit 57d4186 into ericclemmons:master

1 check passed

Details default The Travis CI build passed
@ericclemmons
Owner

v0.3.0 has been tagged, so you're good to go!

@petehunt

Thanks Eric! I had just done manual testing for ignoreMtime, with the way fixtures are set up currently I think it's kind of hard to test. Maybe integrating a way to generate fixtures programmatically would be easiest?

This project rocks :)

@ericclemmons
Owner

Glad you like it! If you can tell me how you manually tested, I can setup the fixture.

And yea, I'm not quite a fan of testing in grunt, and everytime i write new tests its a crapshoot :)

@petehunt

I basically ran it on multiple files, changed one file, and looked at the file mtimes to be sure only the changed file was updated.

@ericclemmons
Owner

Oh, I can build a test around that!

@coveralls

Coverage Status

Changes Unknown when pulling 57d4186 on petehunt:use-transform into ** on ericclemmons:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 38 additions and 19 deletions.
  1. +1 −1  .travis.yml
  2. +4 −2 package.json
  3. +33 −16 tasks/react.js
View
2  .travis.yml
@@ -1,3 +1,3 @@
language: node_js
node_js:
- - 0.8
+ - 0.10
View
6 package.json
@@ -1,7 +1,7 @@
{
"name": "grunt-react",
"description": "Grunt task for compiling Facebook React's .jsx templates into .js",
- "version": "0.2.0",
+ "version": "0.3.0",
"homepage": "https://github.com/ericclemmons/grunt-react",
"author": {
"name": "Eric Clemmons",
@@ -45,6 +45,8 @@
],
"dependencies": {
"react-tools": "~0.3.3",
- "through": "~2.3.4"
+ "through": "~2.3.4",
+ "glob": "~3.2.1",
+ "mkdirp": "~0.3.5"
}
}
View
49 tasks/react.js
@@ -8,17 +8,21 @@
'use strict';
-var Path = require('path');
-var path = Path.dirname(require.resolve('react-tools'));
-var jsx = Path.join(path, 'bin', 'jsx');
-var spawn = require('child_process').spawn;
+var fs = require('fs');
+var glob = require('glob');
+var mkdirp = require('mkdirp');
+var path = require('path');
+
+var visitors = require('react-tools/vendor/fbtransform/visitors').transformVisitors;
+var transform = require('react-tools/vendor/fbtransform/lib/transform').transform;
module.exports = function(grunt) {
grunt.registerMultiTask('react', 'Compile Facebook React .jsx templates into .js', function() {
var done = grunt.task.current.async();
var options = this.options({
- extension: 'js'
+ extension: 'js',
+ ignoreMTime: false
});
this.files.forEach(function(f) {
@@ -35,21 +39,34 @@ module.exports = function(grunt) {
return true;
}).map(function(dir) {
- var source = Path.resolve(dir);
- var target = Path.resolve(f.dest);
- var args = [source, target];
- if (options.extension) {
- args.unshift('--extension=' + options.extension);
- }
+ var source = path.resolve(dir);
+ var target = path.resolve(f.dest);
- if (options.relative) {
- args.unshift('--relative');
- }
+ // Find all of the files to transform.
+ glob(path.join(source, '**', '*.' + options.extension), {}, function(er, files) {
+ files.map(function(srcFile) {
+ var destFile = target + srcFile.substring(source.length, srcFile.length - options.extension.length) + 'js';
+
+ if (!options.ignoreMTime) {
+ var srcStat = fs.statSync(srcFile);
+ var destStat = fs.existsSync(destFile) && fs.statSync(destFile);
+
+ if (destStat && Date.parse(srcStat.mtime) < Date.parse(destStat.mtime)) {
+ return;
+ }
+ }
- var conversion = spawn(jsx, args, { stdio: 'inherit'});
+ var src = fs.readFileSync(srcFile).toString();
+ var newSrc = transform(visitors.react, src).code;
+ var destDir = path.dirname(destFile);
- conversion.on('close', done);
+ mkdirp.sync(destDir);
+ // mkdir
+ fs.writeFileSync(destFile, newSrc);
+ });
+ done();
+ });
});
});
});
Something went wrong with that request. Please try again.