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

keepComments config option doesn't seem to do anything #2206

Closed
rootwork opened this issue Mar 12, 2021 · 8 comments
Closed

keepComments config option doesn't seem to do anything #2206

rootwork opened this issue Mar 12, 2021 · 8 comments

Comments

@rootwork
Copy link

Describe the bug

I know I'm new to this, so maybe I'm just missing something basic?

I've tried passing --config.keepComments false on the command line, and it doesn't seem to have any effect on the output. I thought maybe it only worked if minification was turned on, but that doesn't make any difference. I also tried --config.minifyOptions='{"minifyCSS": true, "keepComments": false}' but the effect was the same.

To Reproduce
Steps to reproduce the behavior:

  1. echo '<mjml><mj-body><!-- comment --></mj-body></mjml>' > index.mjml
  2. mjml -r index.mjml --config.keepComments false -o index.html
  3. grep 'comment' index.html
  4. grep returns <!-- comment -->

Expected behavior

<!-- comment --> should not occur in the file; grep should return nothing.

MJML environment (please complete the following information):

  • OS: Linux (Mint 20.1, Ubuntu 20)
  • MJML Version: 4.9.0
  • MJML tool: npm mjml
@iRyusa
Copy link
Member

iRyusa commented Mar 12, 2021

@kmcb777 It looks like this option is broken on CLI

const html = `
<mjml>
  <mj-body>
    <mj-section>
<!-- Debug comment -->
    </mj-section>
  </mj-body>
</mjml>
`

const mjml2html = require('mjml')

console.log(mjml2html(html, { keepComments: false }))

Removes comment properly

@kmcb777
Copy link
Collaborator

kmcb777 commented Apr 23, 2021

@rootwork This should also be fixed, there was an issue in the cli concerning the whole options object passed to mjml2html

@rootwork
Copy link
Author

Hmm, I've upgraded:

❯ mjml --version
mjml-core: 4.9.1
mjml-cli: 4.9.1

But when I run the steps to reproduce it's still occurring.

kmcb777 pushed a commit that referenced this issue Apr 28, 2021
@kmcb777
Copy link
Collaborator

kmcb777 commented Apr 28, 2021

my bad, this option was actually not handled by the cli
this will be fixed in the next release

@kmcb777
Copy link
Collaborator

kmcb777 commented Apr 30, 2021

@rootwork it should be good now in 4.9.2

@kieranajp
Copy link

kieranajp commented Apr 30, 2021

Hi @kmcb777, this seems to have broken something for us. We run mjml in a CI pipeline, and in the last 15 minutes (so since 4.9.2 release) the pipeline has been failing.

Here's where we run it, as you can see we don't do anything complex:

RUN npm i -g mjml
[...]
RUN ["mjml", "/tmp/messaging-service.mjml", "-o", "/tmp/messaging-service.html"]

This is failing with the following error:

Step 5/17 : RUN ["mjml", "/tmp/messaging-service.mjml", "-o", "/tmp/messaging-service.html"]
 ---> Running in 58cd7b711d46
/usr/local/lib/node_modules/mjml/node_modules/mjml-cli/lib/client.js:176
  }, argv.c.keepComments === 'false' && {
            ^
TypeError: Cannot read property 'keepComments' of undefined
    at _default (/usr/local/lib/node_modules/mjml/node_modules/mjml-cli/lib/client.js:176:13)
    at Object.<anonymous> (/usr/local/lib/node_modules/mjml/node_modules/mjml-cli/bin/mjml:3:28)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Function.Module._load (node:internal/modules/cjs/loader:828:14)
    at Module.require (node:internal/modules/cjs/loader:1012:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at Object.<anonymous> (/usr/local/lib/node_modules/mjml/bin/mjml:4:1)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)

I have locked us to 4.9.1 for now, but thought you might want a heads up.

@kmcb777
Copy link
Collaborator

kmcb777 commented Apr 30, 2021

Hi @kieranajp thanks for reporting this i'll fix this right now

@kmcb777
Copy link
Collaborator

kmcb777 commented Apr 30, 2021

Fixed in v4.9.3

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

No branches or pull requests

4 participants