Skip to content

Enable configuration of harmony support #20

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

Merged
merged 9 commits into from
May 4, 2014
Merged

Conversation

Alxandr
Copy link
Contributor

@Alxandr Alxandr commented May 4, 2014

I was running traceur after react. The fact that react rewrote classes messed the whole thing up.

@Daniel15
Copy link
Member

Daniel15 commented May 4, 2014

This is good for if you call the JSX transformer directly, but how would it be enabled for the ASP.NET JsxHandler (going to a *.jsx file directly in the browser) or for Cassette / ASP.NET Minification? Maybe it's worth adding as a configuration option into IReactSiteConfiguration?

Please fix the build errors as well - The build is failing at the moment since all public parameters require XML documentation comments: http://teamcity.codebetter.com/viewLog.html?buildId=117862&buildTypeId=bt1243&guest=1

Out of curiosity, how did it mess up Traceur? The transforms are fairly simple and definitely shouldn't mess up any other build steps.

@Alxandr
Copy link
Contributor Author

Alxandr commented May 4, 2014

Traceur creates classes on it's own, with it's own handling of prototypes etc. React did things differently. Most of all the code ended up a mess, and traceur wasn't able to do it's transformations as well as it should.

@Alxandr
Copy link
Contributor Author

Alxandr commented May 4, 2014

Updated the tests as well. Though they should probably be changed, as they are testing implementation details.

@Daniel15
Copy link
Member

Daniel15 commented May 4, 2014

Thanks! We still need to work out how to make this configurable for the ASP.NET handler and minification libraries. Maybe just add it to IReactSiteConfiguration.

Could you please also fix the indentation? The ReactJS.NET files use tabs for indentation but your changes are using spaces. Find and replace should be able to fix this :)

@Alxandr
Copy link
Contributor Author

Alxandr commented May 4, 2014

There you go. Added to config, and converted to tabs. Also signed the facebook "I don't care what you do with my code" thingy. Would be much appreciative if this ended on nuget within reasonable short time :)

"/** @jsx React.DOM */ <div>Hello World</div>"
));
"/** @jsx React.DOM */ <div>Hello World</div>",
false
Copy link
Member

Choose a reason for hiding this comment

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

There's still spaces here :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I edited the test-file as well.

@Daniel15
Copy link
Member

Daniel15 commented May 4, 2014

I am planning on doing a minor bugfix release soon so there shouldn't be too much of a delay for the changes to land in a release package (once completed). Packages are pushed to the dev server as soon as the changes land on master, so you could use those NuGet packages until I push a new release (once these changes are merged) 👍

@Alxandr
Copy link
Contributor Author

Alxandr commented May 4, 2014

@Daniel15 Are you available for IM?

{
get
{
return _config;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please pass this in via the constructor of JsxTransformer instead, similar to how all the other dependencies are injected?

@Daniel15
Copy link
Member

Daniel15 commented May 4, 2014

@Alxandr - Sure, I'm normally available on Facebook (http://facebook.com/daaniel) and sometimes on Google Talk (daniel at d15.biz)

Daniel15 added a commit that referenced this pull request May 4, 2014
Enable configuration of harmony support
@Daniel15 Daniel15 merged commit 1c8ea6d into reactjs:master May 4, 2014
@Daniel15
Copy link
Member

Daniel15 commented May 4, 2014

Thanks @Alxandr! Merged into master.

@Daniel15
Copy link
Member

Sorry it took so long to get this into a release! This is in the 1.1.0 release and I've credited you in the blog post at http://reactjs.net/2014/08/1.1-release.html. Thanks again. :)

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