search

facebook / jsx

The JSX specification is a XML-like syntax extension to ECMAScript.
http://facebook.github.io/jsx/
1.96k stars 133 forks source link

Extend JSXAttributeName by { AssignmentExpression } #22

Closed NekR closed 9 years ago

NekR commented 9 years ago

Changes to the spec if #21 will be agreed.

syranide commented 9 years ago

IMHO :-1:

Having to use dynamic property names is indicative of a poorly designed component or bad practices, we don't need a dedicated syntax for it.

sebmarkbage commented 9 years ago

There might be an argument for consistency with object literals here. Since it's valid to do: { [x]: true } and it might be possible to spread that value using the spread operator. So, it's already possible. The purpose for it is symbols. It might be useful to have Symbol keys.

I'm not sure if we would support this in the React transform but I could see this being ok for the broader JSX syntax. Even if we did, we could lint against it. I don't think it's the responsibility of this spec to enforce best-practices.

I don't believe this conflicts with any other interpretation of that syntax. So it's plausible.

@RReverser, what do you think?

RReverser commented 9 years ago

I already wrote short comment on the original issue: https://github.com/facebook/jsx/issues/21#issuecomment-62256096 with "might be possible to spread that value using the spread operator" example.

However, afterwards I was somewhat confused by

Having to use dynamic property names is indicative of a poorly designed component or bad practices

because this makes sense too, and I couldn't recall good cases where I would need computed attributes.

The purpose for it is symbols. It might be useful to have Symbol keys.

This currently seems to be the only useful case for computed attributes, but still not sure how often they will be needed per-instance and not per-class.

However, it might be useful for some implementations or even for us in future, i.e., if we decide to move key and ref from regular properties to symbols so they wouldn't interfere with same-named per-component properties:

import {key as reactKey, ref as reactRef} from 'react/symbols';
var component = <Component [reactKey]="reactKey" key="customProperty" />;

So making it part of extended spec but not of current React's implementation seems to be perfect solution to me.

NekR commented 9 years ago

So making it part of extended spec but not of current React's implementation seems to be perfect solution to me.

Yes, I propose this for spec, not to React itself. Since implementations of JSX already can choose to support some feature or not (namespaces).

@RReverser you are used here square brackets: var component = <Component [reactKey]="reactKey" key="customProperty" />;

So if you will agreed with this feature, you think it should be square brackets like in ES6, but not curly breackets like everywhere else in JSX?

RReverser commented 9 years ago

@RReverser you are used here square brackets:

It was more pseudo-code sample of possible usage. Curly brackets are fine :)

syranide commented 9 years ago

@sebmarkbage

There might be an argument for consistency with object literals here. Since it's valid to do: { [x]: true } and it might be possible to spread that value using the spread operator. So, it's already possible. The purpose for it is symbols. It might be useful to have Symbol keys.

I'm not sure if I'm buying the consistency argument, it's a slippery slope that by extension means that we "must" support <Comp key /> as well (short-object notation) and whatever else JS invents that may or may not actually make sense for components.

But Symbols is a valid point and it might make sense to support it for that reason.

The only thing that sticks out is that Sumbols are not serializeable I believe? I'm not sure if that's actually something we care about anymore, but I know there was talks (quite) some time ago about serializing JSX.

@RReverser

Square bracket syntax seems preferable IMHO :+1: (more descriptive and to disambiguate from {...foo}, not that it is strictly necessary at current though)

RReverser commented 9 years ago

The only thing that sticks out is that Sumbols are not serializeable I believe? I'm not sure if that's actually something we care about anymore, but I know there was talks (quite) some time ago about serializing JSX.

@syranide When we decide to implement it, it can be done with custom serializer support that would recognize and (de-)serialize recognized symbols with simple comparison or ES6 Map.

RReverser commented 9 years ago

It's been half of year already and looks like PR is not mergeable anymore. If we want to get back to it, feel free to open another one.

NekR commented 9 years ago

@RReverser hah, sure it's not mergeable, because I do not monitor this PR every day.

If we want to get back to it, feel free to open another one.

How I can know if you want it?

sebmarkbage commented 9 years ago

Let's keep the discussion going in #21 and we can decide there.