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

Option to force quoted/string properties in transpiled JSX #6812

Open
Treeki opened this Issue Nov 13, 2017 · 5 comments

Comments

Projects
None yet
6 participants
@Treeki

Treeki commented Nov 13, 2017

Currently, Babel's JSX transpilation doesn't really work with the Closure Compiler because of how it transpiles attributes:

	return <button onClick={() => alert('Hi!')}>{props.name}</button>;
	return h(
		'button',
		{ onClick: () => alert('Hi!') },
		props.name
	);

The advanced optimisations in CC cause it to consider onClick to be a property that can be renamed - which isn't the case. So, the resulting optimised code looks like this:

return I("button",{P:function(){return alert("Hi!")}},a.name)

Note that onClick has become P.

It would be really great to have an option that forces quoted properties so that it would instead generate { 'onClick': () => alert('Hi!') }, which CC knows to leave untouched.

I've been able to patch this behaviour in my local copy by modifying the convertAttribute function in babel-helper-builder-react-jsx, but I would much prefer to use an untouched package and simply be able to turn it on with an option in my babelrc.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Nov 13, 2017

Collaborator

Hey @Treeki! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Nov 13, 2017

Hey @Treeki! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 13, 2017

Member

Thanks for the issue @Treeki! Do you know if this is a generic problem for Babel + closure or only with jsx? Just wondering if it has to be an option in the react transform/preset or for other properties.

Member

hzoo commented Nov 13, 2017

Thanks for the issue @Treeki! Do you know if this is a generic problem for Babel + closure or only with jsx? Just wondering if it has to be an option in the react transform/preset or for other properties.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 13, 2017

Member

We've generally had issues with Closure Compiler because it has so many expectations about code formatting that aren't semantically different. The main one we usually hear about is casting types, since we strip out any unnecessary parentheses, and CC uses them for casting.

I don't have a good enough sense of what CC does and does not touch to say if there are other parts of the codebase would cause similar issues to this one.

That said, we don't really make any guarantees like this, so I'd personally say that running CC on Babel's output is questionable. You'd at least one to make sure you've got a lockfile and are running your unit tests on the result after processing, since there's no guarantee that if things do work that we won't accidentally break them if we refactor a transform or something in the future.

Member

loganfsmyth commented Nov 13, 2017

We've generally had issues with Closure Compiler because it has so many expectations about code formatting that aren't semantically different. The main one we usually hear about is casting types, since we strip out any unnecessary parentheses, and CC uses them for casting.

I don't have a good enough sense of what CC does and does not touch to say if there are other parts of the codebase would cause similar issues to this one.

That said, we don't really make any guarantees like this, so I'd personally say that running CC on Babel's output is questionable. You'd at least one to make sure you've got a lockfile and are running your unit tests on the result after processing, since there's no guarantee that if things do work that we won't accidentally break them if we refactor a transform or something in the future.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 13, 2017

Member

To be clear, I have no problem with this as an opt-in flag, I just want to warn that it could be a broader issue.

Member

loganfsmyth commented Nov 13, 2017

To be clear, I have no problem with this as an opt-in flag, I just want to warn that it could be a broader issue.

@xtuc xtuc added the area: jsx label Nov 16, 2017

@arv

This comment has been minimized.

Show comment
Hide comment
@arv

arv May 24, 2018

Contributor

The right/simpler solution is to use an externs file telling Closure Compiler about React's public API: https://github.com/google/closure-compiler/wiki/Externs-For-Common-Libraries#facebook-react

Contributor

arv commented May 24, 2018

The right/simpler solution is to use an externs file telling Closure Compiler about React's public API: https://github.com/google/closure-compiler/wiki/Externs-For-Common-Libraries#facebook-react

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment