support for custom interpolation tags

[bodokaiser]

Hello,

I wanted to use swig templates on for server side rendering and reactive on the client. A problem I had was that reactive does not allow setting own interpolation tags so I needed duplicates.

Example:

<h1>{{ name }}</h1>

reactive.intReg(/\{{ ([^}]+)\ }}/g);
reactive.intChar('{{');

In this pull request I added support for setting a custom regex and character for interpolation (similar to the set, get, .. adapters). There is also a test which covers this specific use case. I guess you may want to change the public api (names).

Best,
Bo

[bodokaiser]

@anthonyshort @visionmedia could someone take a look at this?

[ianstormtaylor]

whats the use case for intChar and intReg? instead of just one way to do it. looks like it might just be a left over from the current hardcoded way.

and then, i'm not a fan of unnecessary abbreviations, i'd probably prefer an api like:

reactive.interpolate('{', '}');

[tarqd]

Definitely feeling an api that accepts

reactive.interpolate('{', '}');
reactive.interpolate(/\{{ ([^}]+)\ }}/g);
reactive.interpolate(function(str) {
  // crazy custom interpolation function here
})

[ianstormtaylor]

problem with the second two is that isInterpolate gets no love to know whether it should interpolate at all

[ianstormtaylor]

and by isInterpolate i definitely meant hasInterpolation

[tarqd]

For regex it could use RegExp#test without the global modifier assuming test doesn't optimize that away. Last case I'd either assume the function takes care of only running if necessary or support a reactive.interpolate(interpolatefn, hasinterpolationfn) interface.

Really though just specifying a begin/end tag would cover most cases imho, I can't imagine needing anything more complex unless you needed some escape rules or something

[bodokaiser]

@ilsken

I would like to omit the support of an interpolation support if this is okay?

[tarqd]

This should do the trick

function removeGlobal(regexp) {
   var flags = regexp.ignoreCase ? "i" : ""
   if (regexp.multiline) flags += "m"
   return new RegExp(regexp.source, flags)

I'm on my phone though so ymmv

[bodokaiser]

This actually should be handled by regex.global = false.

I tested the regex in the console and found out that it always changes from true to false (at least in the chrome console).

Don’t know why.

Am 11.12.2013 um 15:33 schrieb Chris Tarquini notifications@github.com:

I'm on my phone but something like

function removeGlobal(regexp) {
var flags = regexp.ignoreCase ? "i" : ""
if (regexp.multiline) flags += "m"
return new RegExp(regexp.source, flags)
I'm on my phone though so ymmv

—
Reply to this email directly or view it on GitHub.

[tarqd]

Unfortunately RegExp#global is a read-only property, I was suggesting you clone the regexp with the global flag cleared in addition to the user-supplied regex. This way you just use the global-free version for testing and the user supplied one for execution

[tj]

I think keeping it simple to reactive.interpolate('{', '}'); would be more than enough, no reason to go crazy with it, regexps are bad at pairing depth anyway, our current regexps for {} are "incorrect" in that sense, you can't have { foo({ some: 'stuff' }) }, so supplying the chars would be better

[bodokaiser]

@visionmedia should I update the pull request to use something like str.substring(str.indexOf(interpolation[0]), str.indexOf(interpolation[1])) or do you mean a different approach?

[tarqd]

I'd imagine you'd have to count the open/close braces and only create a substring when the count is equal

[tarqd]

@visionmedia Would something like this work?

[tj]

yeah you have to iterate and pair them up

[bodokaiser]

oh shit this looks a bit too mind fucking for me.

Would it be an options to merge the current strategy and may be later update it to the proper way?

Am 12.12.2013 um 18:48 schrieb TJ Holowaychuk notifications@github.com:

yeah you have to iterate and pair them up

—
Reply to this email directly or view it on GitHub.

[tarqd]

Don't be thrown off by that mess I posted, it's not so bad haha. The logic is basically increment on an open brace, decrement on a closed brace and save the stuff in between braces

Result: {foo({bar: true})} other stuff
        ^    ^         ^ ^ 
        1    2         1 0                
        ^              ^ ^
        A              B C

A. Since the count is 1 and this is an open brace - Save the index of the next character (f), anything before this character should be saved as non-interpolated text (Result:)
B. Closed brace but the count greater than 0, that must mean there's still an unclosed pair of outer braces
C. Closed brace, count is now 0, everything from the saved index to the current index (foo({bar: true})) is an interpolated chunk, send it to the callback and save the current index + 1 (the space before the o in other)

Once you're done with all the braces, save str.substr(last_saved_index) to grab any left over text (other stuff) and return the result.

I can just try and integrate this with your pull request when I get home though unless you want to take a whack at it

[bodokaiser]

Okay makes sense :)

I could give it another try tomorrow if you want.

Today is already a bit late ...

Am 14.12.2013 um 16:21 schrieb Chris Tarquini notifications@github.com:

Don't be thrown off by that mess I posted, it's not so bad haha. The logic is basically increment on an open brace, decrement on a closed brace and save the stuff in between braces

Result: {foo({bar: true})} other stuff
^ ^ ^ ^
1 2 1 0
^ ^ ^
A B C
A. Since the count is 1 and this is an open brace - Save the index of the next character (f), anything before this character should be saved as non-interpolated text (Result:)
B. Closed brace but the count greater than 0, that must mean there's still an unclosed pair of outer braces
C. Closed brace, count is now 0, everything from the saved index to the current index (foo({bar: true})) is an interpolated chunk, send it to the callback and save the current index + 1 (the space before the o in other)

Once you're done with all the braces, save str.substr(last_saved_index) to grab any left over text (other stuff) and return the result.

I can just try and integrate this with your pull request when I get home though

—
Reply to this email directly or view it on GitHub.

[bodokaiser]

@ilsken sry but I failed with implementing the new algorithm into reactive (sitting on this nearly 2h till now...) so except you want to take a look and fix it I would give up and close?

[tarqd]

I submitted a pull request on your fork. All the tests are passing so you can just look it over and merge it

[bodokaiser]

I hope it did not took to much time on your side! Thank you.

[bodokaiser]

@visionmedia ready for merge?

[defunctzombie]

Could we maybe get some cleanup in the commits so they can be reviewed better? There is a lot of commits which could really by squashed here.

[bodokaiser]

@defunctzombie I hope I have done this right. If you want I also can wipe out the first commit which includes the "old" custom interpolation way from me.

[defunctzombie]

@bodokaiser much much better :) I think that your two commits could be reverse tho. You could migrate to the updated interpolation algorithm, and then apply the feature/api to support custom tags. Will make your commits even cleaner since adding support for custom tags introduces code which you then delete in the algorithm commit.

[bodokaiser]

@defunctzombie

You mean this on git rebase -i master?

pick ed55f9d updated interpolate alogirthm  
squash 2d8a43e adding support for custom interpolation tags 

[defunctzombie]

@bodokaiser was just an observation about the code changes between the two commits. To me it seems that your "updated algo" commit is logically the first one to make since it makes your custom tags commit simpler. Not sure what sequence of git magic needs to happen to reverse them (maybe some manual editing). In either case, I would wait to see what others say about these changes cause they may have different feedback. I am but just one voice :)

[bodokaiser]

I personally would leave the order as the "update algorithm" commit improves the first one. Though it still may be better for the master commit history.

[bodokaiser]

@ilsken could you review?

[bodokaiser]

@ilsken @defunctzombie @ianstormtaylor sorry for being so pushy but I would really like to use this feature :)

[tarqd]

@bodokaiser Looks good to me but I'm just a user of reactive so getting it merged is up to everyone else haha. I agree though it's a pretty nice feature to have

[defunctzombie]

@anthonyshort @ianstormtaylor I am willing to step up maintain this more actively if there is interest. I currently maintain my own fork with things like iteration, separate adapters, and various other little fixes/tests. Would really like to contribute that back to the project :)

[bodokaiser]

always +1 for reviving a project :)

Am 11.01.2014 um 15:47 schrieb Roman Shtylman notifications@github.com:

@anthonyshort @ianstormtaylor I am willing to step up maintain this more actively if there is interest. I currently maintain my own fork with things like iteration, separate adapters, and various other little fixes/tests. Would really like to contribute that back to the project :)

—
Reply to this email directly or view it on GitHub.

[tarqd]

@defunctzombie I'd be very interested. I'm still itching to use reactive in production. Can you enable issues on it? I have some ideas I'd like to bounce off you

[malaney]

+1 @defunctzombie

[tj]

@defunctzombie added to the org!

[jonathanong]

You should give him npm rights to all the repos so he doesn't have to ping you anymore :)

[tj]

fuck npm

[tj]

we should just have a communal user haha, way easier

[jonathanong]

But then you have to keep switching users :/ unless we all literally publish everything under one account. I'd be down for that.

[vendethiel]

it's a shame you need that to not deal with crazy issues haha :s

[defunctzombie]

Closing since the codebase is changing a bit for reactive 1.0 (next branch) and I am not sure it is realistic to support cross-template-engine'ing a template for reactive with other things since we do not focus on mustache or other common styles. It sounds like you got it to work, but this PR would still need to be cleaned up and wondering if we can't handle it simpler like was done for {