Skip to content

Conversation

@remixz
Copy link
Contributor

@remixz remixz commented May 14, 2014

I implemented this by checking for type="text/jsx;harmony", since this has a bit of a cleaner implementation rather than parsing a JSON object out of a data attribute. If in the future there are other options to pass, it would make sense to move to a system like that.

Along with adding support, there is also a new example added that's the basic-jsx example with harmony syntax. Feedback on implementation totally welcome! 😄

I implemented this by checking for `type="text/jsx;harmony"`, since this
has a bit of a cleaner implementation rather than parsing a JSON object
out of a data attribute. If in the future there are other options to
pass, it would make sense to move to a system like that.

Along with adding support, there is also a new example added that's
the basic-jsx example with Harmony syntax.
@syranide
Copy link
Contributor

Looks good, although I wonder if it's really a good idea to use text/jsx;harmony which would break with the MIME-standard I believe, also it could prevent it from being using in HTTP headers and things like that (not that I think that's every going to make sense).

text/jsxharmony kind of makes sense to me, and would achieve the same thing, @zpao @spicyj ?

@pedroteixeira
Copy link

I believe the spec [1] is ok with having optional parameters, such as: "text/jsx; harmony=true;".
Another common would be something like "text/jsx+harmony" (which is the pattern github follows in its API [2])

[1] http://en.wikipedia.org/wiki/Internet_media_type
[2] https://developer.github.com/v3/

@syranide
Copy link
Contributor

Ah you're definitely right, but unless I'm misinterpreting the spec, there's still kind of a complication as text/jsx; harmony; charset=utf-8 is invalid it seems, it would be text/jsx; harmony,charset=utf-8 or something (not sure what TBH).

text/jsx+harmony makes sense to me at least.

EDIT: Researching a bit more it seems like text/foo+bar is "always" used as being two proper MIME-types i.e application/soap+xml. Does it really matter? Probably not, but it's something.

@zpao
Copy link
Member

zpao commented May 14, 2014

I'm going to shoot down text/jsxharmony - I would like to stick to a single fake mimetype and add options in some other way. I agree that adding a data-options attr is verbose, but really, the use case we're solving here is for toys/mockups, not production code, so I don't feel strongly about how verbose we make this.

text/jsx+harmony seems like a bad route - as @syranide mentioned it's for combined types (or really additional underlying datatype).

text/jsx;harmony seems ok, though it's not clear if parameters can have naked values or if they need an explicit value, so text/jsx;harmony=true seems like the safest bet.

@remixz
Copy link
Contributor Author

remixz commented May 14, 2014

Another option: we could implement it with a data-harmony attribute (so it'd look something like <script type="text/jsx" data-harmony>), which would avoid this mime type stuff. Parsing out JSON from an attribute just seems like more work than needed, but if it's preferred I'll implement it.

@zpao
Copy link
Member

zpao commented May 14, 2014

Nah, let's do text/jsx;harmony=true

@remixz
Copy link
Contributor Author

remixz commented May 14, 2014

Done!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented May 19, 2014

Could you rebase? #1558 made some other changes in there so this doesn't apply cleanly anymore.

@remixz
Copy link
Contributor Author

remixz commented May 20, 2014

@zpao Rebased. Let me know if you want me to squash the commits.

@syranide
Copy link
Contributor

@zpao It just struck me, if we want to do this properly, it should really be text/x-jsx (you didn't register it with IANA did you? ;), but that is apparently also discouraged as of late, in-favor of: text/vnd.jsx (or text/vnd.react.jsx) unless you also get approved by IESG (or similar). http://www.iana.org/form/media-types

Do we care? Don't ask me :)

But it could make the case for something like <script type="text/plain" data-react="jsx?harmony">, since it's dev only anyway, but it is certainly not as sexy!

@zpao zpao merged commit 765ee8e into facebook:master Jun 20, 2014
@zpao
Copy link
Member

zpao commented Jun 20, 2014

Merged! Thanks @remixz and apologies for the delay.

And yes @syranide, technically we should have been text/x-jsx but ugh :)

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.

5 participants