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

Fixed escaper initialization issue #50

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

ilich
Copy link

@ilich ilich commented Nov 19, 2011

Hi Tim,

Thank you for the great project. It is really useful and saved me a lot of time.

Yesterday I found an issue with Haml.render function. For example, if you have a document:
!!!
%html
%head
$title&= title

Then you tries to call Haml.render(doc, { locals: { title: "The test" } }); The call fails with the error of undefined function.

The issue is &= title converted to undefined(title) call what is wrong. I modified the code to initialize escaper function in the render method as well. It has been initialized only in Haml(haml, config) function before.

The second change was to fix how you process end of line characters on Windows. Unit had tests because they expected to have \r\n in a rendered content but there were just \n or \n\n. I fixed the issue: if the platform is Windows then use \r\n otherwise just \n.

I also added a possibility to run only selected tests. For example, node tests.js comments.haml.

I changed deprecated module 'sys' with 'utils' in the tests.js

Please let me know if you have questions or concerns. Otherwise please merge them to the main branch.

Best regards,
Ilya

…d issue with new line on Windows (use \r\n instead of \n when running on Windows). Fixed unit tests. Added posibility to run only some tests from the test kit. Use util module instead of deprecated sys module.
@@ -3,6 +3,9 @@ var Haml;
(function () {

var matchers, self_close_tags, embedder, forceXML, escaperName, escapeHtmlByDefault;

var NEW_LINE_STR = process.platform.match(/^win/i) ? "\\r\\n" : "\\n";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using haml-sprockets in my ruby app and serve haml.js to render my templates on the client. This line causes the following error: 'process is not defined' Is this a Node specific change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's Node specific which represents the running process.

http://nodejs.org/docs/v0.6.5/api/process.html

I would change

var NEW_LINE_STR = process.platform.match(/^win/i) ? "\r\n" : "\n";

to

var NEW_LINE_STR = "'n";

to be able to run the script in a web-browser.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if this pull request is accepted, users would need to manually edit haml.js to be able to continue to use it on the client side?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the good point. I will try to fix this issue to fix the code for web-browsers as well,

thank you for testing. I've been using the library only from node.js.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem :)

I know this isn't strictly the right place to put this...
But I have found that nesting inline javascript in any control structure doesn't work... I am digging in to this but it's a bit of a headache... Have you noticed this?

E.g
:each number in [1,2,3,4]

  • var temp = 1 + number
    #{temp}

@ilich
Copy link
Author

ilich commented Dec 9, 2011

I've created a fix for a web-browser. Please, retest.

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.

None yet

3 participants