Version 1.2 potentially breaks old jade mixins #1435

Closed
leider opened this Issue Feb 26, 2014 · 11 comments

Projects

None yet

2 participants

@leider
leider commented Feb 26, 2014

Hi, it would be great to inform your users that 1.2 may break their code. At least it did for our project (https://github.com/softwerkskammer/Agora)

We had to move some mixins up or down inside the mixing files to make them work again. I couldn't really figure out why.

softwerkskammer/Agora@cba2668 is a version that will not run with jade 1.2. It will fail because it does not resolve "loginMenu" in line 11 of https://github.com/softwerkskammer/Agora/blob/master/views/layoutComponents.jade

Maybe this bit of information helps figure it out.

Cheers!
Andreas

@ForbesLindesay
Member

Which version did it last work in? A lot was broken between v0.35.0 and v1.0.0. Most of these breaking changes were deliberate, although I think slightly more things were broken with mixins than was originally intended. Since then, I'm not aware of any new things which have been broken. Please produce a minimal test case and then I will re-open this issue.

@leider
leider commented Feb 27, 2014

It has been working up to 1.1.15

@leider
leider commented Feb 27, 2014

find this patch here showing what I mean:

diff --git a/test/cases/auxiliary/nested-mixins.jade b/test/cases/auxiliary/nested-mixins.jade
new file mode 100644
index 0000000..ba7aa01
--- /dev/null
+++ b/test/cases/auxiliary/nested-mixins.jade
@@ -0,0 +1,9 @@
+mixin comment
+  p this is a comment with 
+    +foo
+
+mixin list
+  p simplelist
+
+mixin foo()
+  p bar
diff --git a/test/cases/complex-mixins.html b/test/cases/complex-mixins.html
new file mode 100644
index 0000000..3f76fc7
--- /dev/null
+++ b/test/cases/complex-mixins.html
@@ -0,0 +1,11 @@
+<div id="user">
+  <h1>Tobi</h1>
+  <div class="comments">
+    <p>this is a comment with 
+      <p>bar</p>
+    </p>
+  </div>
+</div>
+<body>
+  <p>simplelist</p>
+</body>
diff --git a/test/cases/complex-mixins.jade b/test/cases/complex-mixins.jade
new file mode 100644
index 0000000..ae446d2
--- /dev/null
+++ b/test/cases/complex-mixins.jade
@@ -0,0 +1,12 @@
+include auxiliary/nested-mixins
+
+
+#user
+  h1 Tobi
+  .comments
+    +comment('This',
+            (('is regular, javascript')))
+
+body
+  mixin list()
+
@ForbesLindesay
Member

I'll re-open this for now, but I'm still waiting on a repro case. It should have, a single string of jade that you believe should work, the string of HTML you believe it should generate and either the string of HTML it actually generates or the error message it actually generates. Bonus points if it's fewer than 10 lines of jade.

@leider
leider commented Feb 28, 2014

I thought I'll leave the optimization to you as it is your project. Just apply the patch this will generate new files for your test cases.

It's up to you how you will handle your tests.

@ForbesLindesay
Member

I'm not asking for a test case/optimization. I'm asking you to give me a sample of code that fails. Not a git diff/git patch, not some code that works, not an applicaton that fails. I just want a simple, concise failure case. For an example of what this should look like, see #1112

@leider
leider commented Feb 28, 2014

Ah, I see. Perhaps it is just a misunderstanding. You can execute the patch against this repository and it WILL create the breaking tests in form of jade and associated html files

@ForbesLindesay
Member

Excellent, so do that, create those files, upload them to a repo on github or a gist, and put a link here.

@leider
leider commented Feb 28, 2014

Given a file a.jade containing the following lines

include b

+bang

where file b.jade looks like that

mixin bang
  +foo

mixin foo
  p bar

Will produce the following error

TypeError: test/cases/b.jade:2
    1| mixin bang
  > 2|   +foo
    3| 
    4| mixin foo
    5|   p bar

Object #<Object> has no method 'foo'

Changing file b.jade to

mixin foo
  p bar

mixin bang
  +foo

will fix the error.

Is this a bug or a feature?

@leider
leider commented Feb 28, 2014

Hey, if you remove the blank lines it's much less than 10 lines. What was the price I win?

@ForbesLindesay ForbesLindesay added the bug label Mar 1, 2014
@ForbesLindesay
Member

The prize is that I will try and fix this as soon as possible. Thanks for taking the time to report it.

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