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

Add a 'throwIfNamespace' option for JSX transform #6563

Merged
merged 8 commits into from Oct 29, 2017
Merged

Conversation

@jukben
Copy link
Contributor

jukben commented Oct 26, 2017

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes, close #6535
Tests Added + Pass? Yes
Documentation PR Added
Any Dependency Changes?

Hi guys 👋 , this is my first attempt to contribute, feel free to correct me, I've tried my best. 💪

I'm not sure about this part, to be honest: https://github.com/babel/babel/pull/6563/files#r147231979

P.S: It seems to that tests on master falling. 😔

* If there is flag "throwIfNamespace"
* print XMLNamespace like string literal
*/
return t.stringLiteral(node.namespace.name + ":" + node.name.name);

This comment has been minimized.

Copy link
@jukben

jukben Oct 26, 2017

Author Contributor

I'm not really sure about this, should I make some helper for it? (I've discovered that there is getQualifiedJSXName, so maybe something in this way.)

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 26, 2017

Member

Can a namespace be nested? I don't think a helper is necessary, and you can use a template literal here as well

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 26, 2017

Member

Ok looks like they can't be nested (parse error)

This comment has been minimized.

Copy link
@jukben

jukben Oct 26, 2017

Author Contributor

I don't think that namespace could be nested. Ok, I will go for template literal, I've wanted to stay in style with getQualifiedJSXName implementation. 😆

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 26, 2017

It passes in travis so it's fine, looks like a cache issue or you aren't rebased

@jukben jukben force-pushed the jukben:master branch from 95b57c2 to 0b89e30 Oct 26, 2017
@jukben

This comment has been minimized.

Copy link
Contributor Author

jukben commented Oct 26, 2017

@hzoo You are right. I've rebased it. 👍

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Oct 26, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5508/

Copy link
Member

xtuc left a comment

Thanks for your PR 👍.

We're missing documentation about the new option.

);
if (opts.throwIfNamespace) {
throw path.buildCodeFrameError(
"Namespace tags are not supported. ReactJSX is not XML.",

This comment has been minimized.

Copy link
@xtuc

xtuc Oct 26, 2017

Member

By the way, the error message sounds a bit strange to me. What does ReactJSX is not XML mean? What do you think if we drop this part?

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 26, 2017

Member

Something the react team would of said? They are just saying the React's version of JSX looks like XML (which supports namespace tags) but it's not

We can reword it if you think that makes sense. "React's JSX doesn't support namespace tags (although it looks similar to XML which does)"

This comment has been minimized.

Copy link
@xtuc

xtuc Oct 26, 2017

Member

Yes, I would prefer something like that.

This comment has been minimized.

Copy link
@jukben

jukben Oct 27, 2017

Author Contributor

I tried something like this, feel free to reword this. 👍

@babel babel deleted a comment from babel-bot Oct 26, 2017
@jukben

This comment has been minimized.

Copy link
Contributor Author

jukben commented Oct 27, 2017

@xtuc Sorry! I've added documentation in the last commit.

@xtuc
xtuc approved these changes Oct 27, 2017
Copy link
Member

existentialism left a comment

Small doc nit


`boolean`, defaults to `true`.

This option controls how to handle XML Namespaces. React's don't support it thus default behavior is to throw an error when XML namespace occurs, this behavior could be suppressed by setting it to false.

This comment has been minimized.

Copy link
@existentialism

existentialism Oct 27, 2017

Member

Slight grammar nit:

React's don't -> React's JSX doesn't

Or maybe even:

Toggles whether or not to throw an error if a XML namespaced tag name is used. For example:

<f:image />

Though the JSX spec allows this, it is disabled by default since React's JSX does not currently have support for it.

This comment has been minimized.

Copy link
@jukben

jukben Oct 27, 2017

Author Contributor

@existentialism Thanks for the grammar 😥 . At the end, I've used your version. Providing an example is a good idea.

jukben added 6 commits Oct 26, 2017
@jukben jukben force-pushed the jukben:master branch from 3c48293 to e42fa00 Oct 27, 2017
if (opts.throwIfNamespace) {
throw path.buildCodeFrameError(
"Namespace tags are not supported by default. React's JSX doesn't support namespace tags." +
"You can turn on the 'throwIfNamespace' flag to bypass this warning.",

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 27, 2017

Member

Does the template literal not work here? looks like this is all one line, and you need a space tags.You

This comment has been minimized.

Copy link
@jukben

jukben Oct 28, 2017

Author Contributor

@hzoo You are right. I'm missing a space, good catch. I've used template literals, but then I run into "line is too long' warning. But now I discovered that there is line continuation (\) so I'm going to rewrite this.

@@ -78,7 +78,8 @@ With options:
{
"plugins": [
["@babel/transform-react-jsx", {
"pragma": "dom" // default pragma is React.createElement
"pragma": "dom", // default pragma is React.createElement
"throwIfNamespace": false // default throwIfNamespace is true

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 27, 2017

Member

I think you can just say defaults to true

jukben added 2 commits Oct 28, 2017
@hzoo hzoo merged commit 04d2c03 into babel:master Oct 29, 2017
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.95% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 29, 2017

Awesome, nice work @jukben! 👌

@DanielRuf

This comment has been minimized.

Copy link

DanielRuf commented Oct 29, 2017

Awesome, nice work @jukben! 👌

Definitely, and very fast and great quality =)

@lock lock bot added the outdated label May 1, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.