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

tmpl.js - complete rewrite #941

Closed
wants to merge 6 commits into from
Closed

tmpl.js - complete rewrite #941

wants to merge 6 commits into from

Conversation

aMarCruz
Copy link
Contributor

@aMarCruz aMarCruz commented Jul 9, 2015

@tipiirai, @GianlucaGuarini, @cognitom, @rsbondi

This days I spent some time cleaning and testing tmpl.js, and reviewing its logic.
This is the result.

I wanted above all: backward compatibility, correct details that have generated bugs & issues,
provide flexibility in the implementation, and facilities to understand the code through multiple comments; all without the code grew ...too
(the minified size of this version is 4,386 bytes vs 1,887 bytes of the one in the master branch).

But I have not had time to study other parts of riot, and how they interact with tmpl,
so perhaps tmpl caller filters out characteristics that this code implements, and
therefore has grown unnecessarily.
EDIT: In testing with some issues, I think there are details in the compiler that cause these issues. The string received from tmpl is filtered, new characteristics are not so usefull, and in other cases, the output of tmpl is ignored.

This file passed all riot tests and, in my opinion, is in beta phase, ready to test for
performance and in real environments, so please, break the code now.
I'm test it in latest IE, FireFox, and Chrome. I will do some mobile tests later.

Code is heavily commented 'cause today, we know what we doing, tomorrow... God knows?
#784 is fixed
#744 is related to the compiler

Thanks.

@tipiirai @GianlucaGuarini @cognitom @rsbondi , hi

Last days I spent some time cleaning and testing tmpl.js, and reviewing its logic.
This is the result.

I wanted above all: backward compatibility, correct details that have generated bugs & issues,
provide flexibility in the implementation, and facilities to understand the code through multiple comments; all without the code grew ...too
(the minified size of this version is XXX bytes vs XXX bytes of the one in the master branch).

But I have not had time to study other parts of riot, and how they interact with tmpl,
so perhaps tmpl caller filters out characteristics that this code implements, and
therefore has grown unnecessarily.
EDIT: In testing with some issues, I think there are details in the compiler that cause these issues. The string received from tmpl is filtered, new characteristics are not so usefull, and in other cases, the output of tmpl is ignored.

This file passed all riot tests and, in my opinion, is in beta phase and ready for testing on
performance and in real environments, so please, break the code now.
I'm test it in latest IE, FireFox, and Chrome. I will do some mobile tests later.

Code is heavily commented 'cause today, we know what we doing, tomorrow... God knows?
(Corrections are welcomed, the English is not my friend)

#784 is fixed
#744 is related to the compiler

Thanks.
silly ampersand :[
Escaping the `'/'` char in te result of RegExp.scape is browser dependent.
@GianlucaGuarini
Copy link
Member

this will be merged after the riot 2.2.2 release. Great job @aMarCruz !

@mwielondek
Copy link
Contributor

Kudos 👏

@aMarCruz
Copy link
Contributor Author

Switch PR to reduced version of new-tmpl-2 branch

@aMarCruz aMarCruz closed this Jul 11, 2015
@aMarCruz aMarCruz deleted the new-tmpl-1 branch July 11, 2015 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants