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

Support not-XML literals #7438

Merged
merged 10 commits into from Oct 9, 2018

Conversation

Projects
None yet
6 participants
@Simn
Copy link
Member

Simn commented Sep 18, 2018

See #7035 for discussion.

See InlineXml.unit.hx for examples.

I had to add a hacky boolean argument to format_string. The problem is that it assumes '-delimited strings, which means that all our offsets are off-by-one because the delimiter on these is not part of the actual string. With that, completion works:

2018-09-18_08-54-36

I didn't add this haxe.XmlString type mentioned in #7035 because I don't really see the point. It seems fine to just treat these literals as normal strings. Macros will check for @:markup <string>content</string> anyway.

Probably needs some cleanup, but let's merge it first.

@Simn Simn added this to the Release 4.0 milestone Sep 18, 2018

@back2dos

This comment has been minimized.

Copy link
Member

back2dos commented Sep 18, 2018

I didn't add this haxe.XmlString type mentioned in #7035 because I don't really see the point.

The point is that on the off chance that you're using this for say React, simply having the thing be a String means that it will be accepted as such and therefore you wind up with html escaped markup in your dom, without the compiler telling you.

Example:

class Fancy {
  static public function button()
     return <button>Fancy!</button>;
}

class Foo extends ReactComponent {
  function render() 
    return <div class="foo">${Fancy.button()}</div>
}

Because no macro runs on Fancy.button() it just returns a String and that's a valid child in React, you get no error, you just get non-sense. A separate abstract with @:to String would lead to a type error, because this cast would not apply: https://github.com/kLabz/haxe-react/blob/next/src/lib/react/ReactNode.hx#L20

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 18, 2018

But couldn't the macro that processes the outer content do this check?

My concern here is that this is quite domain-specific and an approach like that would lead to variance problems that don't make a whole lot of sense from Haxe's point of view.

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Sep 18, 2018

But couldn't the macro that processes the outer content do this check?

I think because the metadata @:markup is not "returned" to the call site, so it is not possible to distinguish a plain string vs markup

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 18, 2018

But if the idea is that you don't want a String there, then surely your macro could just check for String and make sure that you return YourString, which can be anything you like.

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Sep 18, 2018

No, we want to accept both String and Markup there. And we need to distinguish that.

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 18, 2018

So you would accept "<button>Fancy!</button>" but not <button>Fancy!</button>? I'm trying to understand the thinking here because it seems to me like you should always validate your strings, no matter how they were delimited...

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Sep 18, 2018

Taking Juraj's example:

<div class="foo">${Fancy.button()}</div>

Here we assumes that the macro understands ${} is wrapping a haxe expression:

If Fancy.button() returns a string, it will be placed as TextNode (with proper escaping) inside div#foo, so that in the browser one will see literally the text <button>Fancy!</button>

If Fancy.button() returns a markup (and the macro knows that), the macro will handle it and put a ButtonNode inside div#foo rendering an actually button in the browser.

The key point here is that plain string itself is a valid node and the macro has no way to know if the user is intended to put a piece of plain text or a button node because the @:markup meta is already gone when the macro sees the string, the macro only sees Fancy.button is a function that returns String.

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Sep 18, 2018

While it compiles, I think the main problem is that it will lead to users' surprises.
I think the original proposal's "reject by typer if not handled by macro` tries to prevent this. But I will let @back2dos explains it from now on.

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 18, 2018

What I was wondering about is why you can't just check string.charAt(0) == "<", but I gather you might want to have escaped text nodes in such a situation...

@ncannasse

This comment has been minimized.

Copy link
Member

ncannasse commented Sep 18, 2018

@Simn that's a very good way to allow for HTML injection everytime a user post a comment starting with < ;) So yes I think there is a need to differentiate these Strings.

However this only works if we do guarantee that XmlString are properly XML formed : we don't want this check at parsing time but we might want it at typing time.

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 18, 2018

@Simn that's a very good way to allow for HTML injection everytime a user post a comment starting with < ;) So yes I think there is a need to differentiate these Strings.

That happens at a whole other level...

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Sep 18, 2018

@Simn that's a very good way to allow for HTML injection everytime a user post a comment starting with < ;) So yes I think there is a need to differentiate these Strings.

I guess we are discussing a compile-time feature?

If the outer macro sees a String (it only sees the type, not value), it will be accepted because String is a valid value at the position.

However if it sees MarkupString, the macro will reject it and say "hey you should process that piece of stuff with a macro first"

@back2dos

This comment has been minimized.

Copy link
Member

back2dos commented Sep 18, 2018

The basic scenario is this: some developer decided to factor out Fancy.button from its call site to extra function, because it's more practical. By mistake they put it somewhere, where no macro is running (i.e. Fancy is not a ReactComponent, so there's no build macro). Instead of the intended vdom for a button, the function now creates a plain string. Upon compiling and reloading the application, the developer is delighted to see that where there's supposed to be a button, there's now some text containing the markup to create the button. The developer begins to wonder if they should just ditch their hopes for static typing and switch back to JavaScript.

The best behavior in this scenario would be to not have default semantics, as I had proposed. In that case, the compiler error would occur where it's most useful: in Fancy.button.

The worst behavior is to just return a String.

A compromise is to generate a different type, so that you at least get some kind of type error, even though it takes a while to figure out what's going on.

But I'm still not convinced about having these default semantics, especially if it's the ones that have been decided on, because they are useless.

We could agree on some JSX-ish semantics, that use markup to ultimately describe a call tree, but that would mean agreeing 1. on syntax and 2. on how exactly that tree is to be built. To reach such consensus will take time that we don't have, if this is to be in Haxe 4. Hence for me the sanest option is to make this a compiler error and once we have such an agreement, use that for the default semantics and offload all that into the compiler.

@Simn Simn referenced this pull request Sep 21, 2018

Merged

Inline Markup Literals #26

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 23, 2018

Hence for me the sanest option is to make this a compiler error and once we have such an agreement, use that for the default semantics and offload all that into the compiler.

I'd be okay with that. However, it would technically still be a breaking change because someone might try-catch the compiler error in a macro. So maybe we should make this a fatal error.

IMO the most convincing argument here is "because they are useless". We've added the interpolation semantics "because we can", but I don't see how this improves anything in any way.

@ncannasse

This comment has been minimized.

Copy link
Member

ncannasse commented Sep 26, 2018

I don't think they are useless at all, let's say you want to write some html templates in Haxe, or have to feed some library with XML, you can now directly use the language for that with interpolation.

@back2dos

This comment has been minimized.

Copy link
Member

back2dos commented Sep 26, 2018

You want to write templates, but don't need loops and don't care about the validity of your markup ... seems a bit of a narrow use case to me ...

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Sep 27, 2018

Let's disallow it at least for the time being. We can always enable it in 4.1 after giving it some more thought. That would also allow us to think about a more fancy processing than just string interpolation.

@Simn Simn merged commit 22ccaa1 into HaxeFoundation:development Oct 9, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@Simn Simn deleted the Simn:not_inline_xml branch Oct 9, 2018

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Nov 30, 2018

This is such an interesting feature :)
We used to read component templates from a separate html/xml file but being able to do this inside the class just makes a lot of sense and keeps everything neatly close together inside the same class.

What I would really appreciate is to have something like the Razor engine on ASP (https://www.w3schools.com/ASP/razor_intro.asp) where you can combine code with xml to create a complex xml structure. With some haxe magic I got this to work pretty well:

<div>
    var count = 20;
    for(i in 0...count)
    {
        if(i == 0)
        {
            <div/>;
        }
        else
        {
            <span/>;
        }
    }
</div>;

Anyone else has some cool ideas / experiences with this?

@kevinresol

This comment has been minimized.

Copy link
Contributor

kevinresol commented Nov 30, 2018

There is the erazor templating engine. But I think it does not support inline xml yet.
You can also have a look at tink_hxx.
Anyhow, such functionality should be provided by a library, not from haxe compiler or the std lib

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Dec 7, 2018

Did some more work in this and one thing that is bugging me is the following. It is mostly a workflow / visual pleasing thing but it might make sense to others as well.

I really dislike the fact that you need to end a piece of xml with a ;
Obviously this is correct because you are actually typing a string, but it just looks really weird. Especially if you consider the context in which it is being used: writing templates. The example below shows that things start to look really complex if you use siblings or nested elements.

Anything we can do about this?

if(i == 0)
{
    <div/>;
    <div/>;
}
else
{
    <div>
        <span><span>
    </div>;
}
@back2dos

This comment has been minimized.

Copy link
Member

back2dos commented Dec 8, 2018

Actually, that's probably a good idea. @Simn: could would we not use expr_next on XML literals and thus make the semicolon optional?

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Dec 8, 2018

Are we sure we don't want to support something like <printf $s $i>("foo", 12)?

@back2dos

This comment has been minimized.

Copy link
Member

back2dos commented Dec 8, 2018

Hmm, personally I'd say if I ever went for anything like that I'd prefer a syntax that keeps modifier and expression closer together, e.g. <printf $s{"foo"} $i{12}>. It's easier to track things and it's also roughly in line with reification syntax.

But then again I'm sure there will be that one person who's world will collapse if the closest they can get to the syntax you brought up will be (<printf $s $i>)("foo", 12).

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Dec 8, 2018

I would say the comment from @back2dos is quite valid: most template systems also write variables inside the xml. Take for example a basic React case:
return <div>Hello, { targetOfGreeting }!</div>;

@benmerckx

This comment has been minimized.

Copy link
Contributor

benmerckx commented Dec 8, 2018

Are we sure we don't want to support something like <printf $s $i>("foo", 12)?

Slightly unrelated, but wouldn't you have to close it like <printf $s $i />(..) for this to be valid?

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Dec 10, 2018

@Simn just wondering how decisions are made for topics like this? Right now it feels like an open topic that floats in my head. Anything I can do to help?

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Jan 2, 2019

Gentle nudge to this thread:

  • Can we expect this to change?
  • Can we make this change ourselves to experiment with it?
@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 2, 2019

I think it's fine to change it like @back2dos suggested.

@ncannasse

This comment has been minimized.

Copy link
Member

ncannasse commented Jan 2, 2019

I agree that ; should not be mandatory in above examples, but I don't think not having expr_next will be enough for that.

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 2, 2019

It's not, we would have to accept > as a pseudo-semicolon like we do for }.

@ncannasse

This comment has been minimized.

Copy link
Member

ncannasse commented Jan 2, 2019

Well, more exactly a literal-XML-terminator, but yes

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Jan 15, 2019

Sorry guys to bump this thread again. I would like to pitch in and help if this is appreciated. I am just quite unfamiliar with this part of Haxe and not sure if and how I can help.

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 15, 2019

I'll handle it!

Simn added a commit that referenced this pull request Jan 15, 2019

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 15, 2019

That should do it. I had to approach this differently than I thought, but it should be cleaner like this anyway.

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Jan 15, 2019

Thanks @Simn! What do you recon is the best way of giving this a try. Should I use the nightly builds or wait until preview 6 comes out?

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 15, 2019

Well it shouldn't be too long till our next release, but I'd still appreciate some testing on a nightly build!

@remcohuijser

This comment has been minimized.

Copy link

remcohuijser commented Jan 17, 2019

Tried it today with the latest nightly and works great 👍

@Simn

This comment has been minimized.

Copy link
Member

Simn commented Jan 17, 2019

Thanks for the feedback!

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