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.

Sign up for GitHub

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

Jump to bottom

Silently fails with HTML comments #79

Closed
pcraig3 opened this issue Apr 1, 2019 · 6 comments · Fixed by #84
Closed

Silently fails with HTML comments #79

pcraig3 opened this issue Apr 1, 2019 · 6 comments · Fixed by #84
Labels
discussion Discussions and proposals question Further information is requested

Comments

@pcraig3
Copy link

pcraig3 commented Apr 1, 2019 •
edited
Loading

Hi there,

Neat library! I really like developing components but don't like shipping large bundle files to the browser. For simple apps, it would be nice to do server-side components that get rendered as strings.

I've been playing around with htm recently (inspired by @timarney's htm-ssr-demo), and got a pretty minimal proof-of-concept up and running. (Most of the code is from a preact-render-to-string example.)

package.json 
{
  "name": "ssr-htm-hello-world",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "express": "^4.16.4",
    "htm": "^2.1.1",
    "preact": "^8.4.2",
    "preact-render-to-string": "^4.1.0"
  },
  "scripts": {
    "start": "node index.js"
  }
}
index.js 
const express = require("express");
const { h, Component } = require("preact");
const render = require("preact-render-to-string");
const htm = require("htm");

const html = htm.bind(h);

const Page = ({ name }) => {
  return html`
    <div class="page">
      <h1>${name}</h1>
      <p>This page is all about ${name}.</p>
    </div>
  `;
};

const app = express();

app.get("/:page", (req, res) => {
  let markup = render(
    html`
      <${Page} name=${req.params.page} />
    `
  );
  res.send(`<!DOCTYPE html><html><body>${markup}</body></html>`);
});

app.listen(3000);

So that works well enough to prove the concept.

However, if I change Page to this:

const Page = ({ name }) => {
  return html`
    <div class="page">
      <!-- html comment -->
      <h1>${name}</h1>
      <p>This page is all about ${name}.</p>
    </div>
  `;
};

then I just get a white page, and if I open devtools, I see <undefined></undefined> in the markup.

There's no warning message or anything, so I'm wondering

  • if this is a bug
  • if I'm using this library in a way it's not supposed to be used
  • or if there's any eslint config or anything that I can set up to catch this kind of thing

Any help or suggestions would be appreciated! I'd like to build a little prototype using htm, but if it fails silently on syntax issues then it probably won't scale very well once you started nesting components inside each other.
@developit
Copy link
Owner

developit commented Apr 7, 2019 •
edited
Loading

I think @jviide has been toying with this. Since comments aren't supported in JSX, we didn't feel the need to support them in HTM.

Not sure if this is old news, but you can use standard JSX style comments in HTM:

const Page = ({ name }) => {
  return html`
    <div class="page">
      ${/* a standard JS comment */}
      <h1>${name}</h1>
      <p>This page is all about ${name}.</p>
    </div>
  `;
};

The benefit of doing so is that they'll be stripped from your JS bundle when you do a production build, and they have no cost at runtime (they evaluate to undefined).

@developit developit added discussion Discussions and proposals question Further information is requested labels Apr 7, 2019
@natevw
Copy link

natevw commented Apr 9, 2019 •
edited
Loading

This has gotten me in the past too; took a while to track down the first time it happened!

Note that @developit's "standard JSX" workaround might be okay when compiled somehow, but in direct JS usage an empty block like `${/* _ */}` is apparently a syntax error [makes Chrome unhappy at least] so I've been resorting to something like ${/* a JSX comment with a JS workaround */null} when needed.

@cj
Copy link

cj commented Apr 19, 2019

@developit If i do a JSX style comment I get Uncaught SyntaxError: Unexpected token }. I can put together an example later.

I think it would be nice to support <!-- html comment -->. If it's no longer supported and support for it is not coming back, we should update the main image in the README.md as it uses html comments.

@natevw
Copy link

natevw commented Apr 19, 2019

Parsing HTML comments seems the cleanest solution to me as well, assuming it can be done without too much extra code.

With JSX (and the earlier E4X ;-) the philosophy is to handle the syntax needs in "code", whereas with htm they happen in markup. Case in point: <Component {...spread} /> vs. <${Component} ...${spread} />. The string (markup) parts are always in control of htm with/without pre-compilation, and since there's already a standard for it there it seems reasonable to make this another "known differenceimprovement" over JSX.

@jviide jviide mentioned this issue Apr 19, 2019
Merged
@jviide
Copy link
Collaborator

jviide commented Apr 19, 2019 •
edited
Loading

Opened PR #84 that suggests one way to support comments.

@jviide
Copy link
Collaborator

jviide commented Jul 29, 2019

PR #84 has now been merged and released as a part of HTM 2.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion Discussions and proposals question Further information is requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for HTML comments
5 participants
@cj @developit @natevw @pcraig3 @jviide