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

Prevent DOM tag name renaming by closure compiler in advanced mode #420

Closed
wants to merge 1 commit into from
Closed

Prevent DOM tag name renaming by closure compiler in advanced mode #420

wants to merge 1 commit into from

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Oct 12, 2013

Without this change tag names will be changed by closure compiler: https://monosnap.com/image/1uZUvJewUyMLRvXm6nHXNL02z.png

form and textarea are ok by default (you can see form in screenshot above).

I'm not sure this is the best way to handle renaming, but I couldn't invent anything better - I wonder if it's possible to get renamed equivalent in runtime...

@SanderSpies
Copy link
Contributor

Alright, that's unfortunate.

And how about using Google Closure Compiler externs? https://developers.google.com/closure/compiler/docs/api-tutorial3?hl=nl&csw=1#externs <- seems like the best solution at first glance.

Note: not a Google Closure expert here.

@piranha
Copy link
Contributor Author

piranha commented Oct 12, 2013

Externs are for the case when you're compiling library for external consumer, but when you're compiling it together with application you want to live without them. Plus they didn't work for me when I compiled them together, haha. :)

@sophiebits
Copy link
Collaborator

Perhaps we should write 'a': false and change the transform to output React.DOM['a'] instead of React.DOM.a

@piranha
Copy link
Contributor Author

piranha commented Oct 12, 2013

But then you'll have to require that people which are not using JSX to write in the same way... I was planning to use React from ClojureScript and import functions just as functions. I could have a simple wrappers for them, of course, not a big deal, but I think that having method name and tag name separately is not bad. :) Plus it's not making code that much bigger it seems to me...

Though yeah, I shivered a bit because of non-DRYness of code, but managed to convince myself it's ok by using method name/tag name argument. :)

@zpao
Copy link
Member

zpao commented Oct 14, 2013

I'm going to be inclined to say no to anything that forces React.DOM['a'] though I might be able to get on board if that's optional in the transform for GCC compat.

This doesn't add that much to the build, but if we take it, it would be 10% of the increase over 0.4. I'm inclined to leave this be until after we ship 0.5. I'd like to get @jordwalke to take a look and see if he has any other crazy ideas here.

   raw     gz Compared to last run
 +1259   +413 build/react.js
 +1108   +405 build/react.min.js

@steida
Copy link

steida commented Oct 23, 2013

@piranha
Copy link
Contributor Author

piranha commented Oct 24, 2013

Thanks, I know about them and used them, but that is something I would love to live without. :)

@jordwalke
Copy link
Contributor

Wasn't there an argument to the GCC command line that allows you to add additional whitelist/blacklists? That adds no overhead to the actual code.

@piranha
Copy link
Contributor Author

piranha commented Nov 25, 2013

Well yeah, it's called 'externs', I guess it could be used, I'm just not sure if that is a good solution to a problem.

@swannodette
Copy link

+1. Would love to just use React from ClojureScript instead of writing something like it from scratch - and I'm sure other folks using Google Closure Compiler in their pipeline would be happy to benefit too.

@sophiebits
Copy link
Collaborator

Why aren't externs appropriate for this case?

@swannodette
Copy link

Actually sorry I didn't quite understand what this pull request was about - I came across it in a fairly roundabout fashion. It does in fact seem to me now that externs are the correct solution.

@piranha
Copy link
Contributor Author

piranha commented Nov 26, 2013

Well biggest problem with externs is that they will have to be specified in every project which depends on React. I wonder if it's possible to annotate code somehow so that Closure Compiler won't rename keys of this particular object...

@steida
Copy link

steida commented Nov 26, 2013

I don't quite understand too. Which object? I am using React with Compiler in advanced mode with no problems.

@piranha
Copy link
Contributor Author

piranha commented Nov 26, 2013

Do you use externs?

@steida
Copy link

steida commented Nov 26, 2013

Sure.

@piranha
Copy link
Contributor Author

piranha commented Nov 26, 2013

That's exactly what I would like to not do. Externs serve for libraries included from elsewhere and I would like to compile React together with my application so that Closure Compiler can perform proper dead code elimination.

@steida
Copy link

steida commented Nov 26, 2013

I compiled it without externs. It wasn't easy, common js stuff etc., and result is: React code produces plethora warnings. I don't want to mess my code with that so I decided to use React as is. Check: https://github.com/steida/este-library/blob/master/Gruntfile.coffee#L108.

@steida
Copy link

steida commented Nov 26, 2013

Btw, React has 25KB gzipped roughly. That's fine. Non trivial app will leverage gzip even more.

@piranha
Copy link
Contributor Author

piranha commented Nov 26, 2013

It surely emits quite a bit of warnings, but nothing serious and it works, if compiled like that:

https://github.com/piranha/pump/blob/master/Makefile#L32

It's not only about size for me, but also a bit about convenience...

@piranha
Copy link
Contributor Author

piranha commented Dec 16, 2013

Anyway, this pull request is not relevant anymore, plus pump needs to be changed to leverage advanced-compiled React (and focus shifts to om anyway, which works with React a bit differently).

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.

7 participants