sighmon / mjml-rails

MJML + ERb/Haml/Slim view template
https://mjml.io
Other
295 stars 64 forks source link

MJML 4.8.x broke my validator again :) #79

Closed denny closed 3 years ago

denny commented 3 years ago

I don't have time to dig into this one as thoroughly as last time, sorry πŸ™ I have a nasty feeling it may be filed under 'feature not a bug' at the MJML end so I'll have to adapt somehow, but I thought I'd run it past you as it is a change in behaviour of/via your gem, without your code having changed, which I would imagine is quite frustrating!

Layout: plugins/ShinyNewsletters/app/views/layouts/newsletter_mailer.html.mjml Partial: spec/fixtures/TEST/views/shiny_newsletters/newsletter_mailer/an_example.html.mjml Validator: app/validators/mjml_syntax_validator.rb Test: plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb#L44

Up to MJML 4.7.1 the validator passed and now it doesn't. If I move the layout code into the partial, then the validator passes, so it seems like either it was wrapping the partial in the layout before and isn't now, or it was less strict about templates being a whole document?

If this is just a case of my needing to tweak a config setting somewhere to allow partials to render (and you could point me at it!) that would be great πŸ˜„ Otherwise, I've pinned MJML to 4.7.1 for now and hopefully I'll have more time to dig into this again late February/early March.

denny commented 3 years ago

https://github.com/mjmlio/mjml/commit/1ee581fd9443aaf7bd7f0aaeca02419cdc148124 looks possibly relevant... :-\

denny commented 3 years ago

Definitely a change in how the mjml command responds to a partial input file πŸ˜’

denny@rocinante:~/ShinyCMS$ cat partial.mjml 
<mj-section>
  <mj-column>
    <mj-text>
      Hello world.
    </mj-text>
  </mj-column>
</mj-section>
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml --version
mjml-core: 4.7.1
mjml-cli: 4.7.1
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml -v partial.mjml 
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml partial.mjml > /dev/null
denny@rocinante:~/ShinyCMS$ yarn install
yarn install v1.22.5
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 2.85s.
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml --version
mjml-core: 4.8.0
mjml-cli: 4.8.0
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml -v partial.mjml 
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml partial.mjml > /dev/null
File: partial.mjml
TypeError: Cannot read property 'replace' of undefined

Command line error:
Input file(s) failed to render
denny@rocinante:~/ShinyCMS$ 
denny commented 3 years ago

Possibly interesting...

denny@rocinante:~/ShinyCMS$ cat invalid_partial.mjml
<mj-section>
  <mj-column>

     <mj-title>
      Titles don't belong in the body block
    </mj-title>

  </mj-column>
</mj-section>
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml -v invalid_partial.mjml
Line 4 of /home/denny/code/denny/ShinyCMS-ruby/invalid_partial.mjml (mj-title) β€” mj-title cannot be used inside mj-column, only inside: mj-attributes, mj-head

Command line error:
Validation failed
denny@rocinante:~/ShinyCMS$ ./node_modules/.bin/mjml  invalid_partial.mjml > /dev/null
File: invalid_partial.mjml
TypeError: Cannot read property 'replace' of undefined

Command line error:
Input file(s) failed to render

... so the explicit validation option -v fails (with a useful error) if the MJML in a partial input file is actually invalid, but not if it's valid-but-incomplete. Is there any way for me to get at that behaviour through mjml-rails currently?

denny commented 3 years ago

I rewrote my validator to call out to mjml --validate directly, which solves the problem for me.

I'm going to close this issue as I feel like the validation/validator stuff is probably an edge case πŸ™‚ If anybody else runs into a related problem then hopefully they'll find this and they can link/re-open.

I was wondering whether to try to add a .validate method alongside .render in Mjml::Parser (again, using mjml --validate to do the actual work). Let me know if that's a feature you'd like to add and I'll take a swing at it if I get time.