jedireza / hapi-react-views

:package: A hapi view engine for React components
MIT License
231 stars 33 forks source link

Added ability to turn off jsx compilation if not required #19

Closed abritinthebay closed 9 years ago

abritinthebay commented 9 years ago

Adds ability to turn off jsx compilation if not required - for example if using babel's require hook or writing in ES5 style React.

This is especially useful as node-jsx is super slow and it eliminates the need for this when it's not required.

jedireza commented 9 years ago

Thanks for opening a PR. This seems reasonable. We should get the tests passing again though.

abritinthebay commented 9 years ago

Ah, agreed. Looks like you have it failing at less that 100% coverage? That's... impressive :)

Will update tests. Not too familiar with Lab but it looks pretty standard.

abritinthebay commented 9 years ago

Ok, updated the tests and I moved the jsxCompile check into the runtime so it's a) easier to test and b) more flexible.

That said - because of the way node-jsx works once it's loaded... it's loaded. That's why the tests are in that order as well. It needs to test those first otherwise it can't test a failure state.

abritinthebay commented 9 years ago

Let me know if there are any more changes you'd like to see.

jedireza commented 9 years ago

Will do. Apologies for the delay, I got slammed after we last chatted.

abritinthebay commented 9 years ago

No worries - been there... repeatedly.

jedireza commented 9 years ago

I sent a minor PR to your fork. If you merge that, the changes will show up here.

jedireza commented 9 years ago

Ok cool. Let's squash the commits into one and I'll merge.

abritinthebay commented 9 years ago

Done and done :)

jedireza commented 9 years ago

Hmm. Not sure about that 3.0.1 commit that got in here.

abritinthebay commented 9 years ago

I think that's just because of your PR. It does list you as the author of it so... no idea.

abritinthebay commented 9 years ago

Looks like the result of an npm patch that isn't in master maybe?

jedireza commented 9 years ago

Maybe you re-based one commit too many? The 3.0.1 is the latest commit on master (1ed5b14f).

abritinthebay commented 9 years ago

I just rebased. As it's my fork it shouldn't affect yours, and will just merge.

jedireza commented 9 years ago

Great. Thank you! This looks good. I'm in a meeting right now, but I'll do a final review later to double check and merge.