glimmerjs / glimmer-vm

MIT License
1.13k stars 188 forks source link

Preprocessing into AST doesn't work with <!DOCTYPE> tags #870

Open timlindvall opened 5 years ago

timlindvall commented 5 years ago

Hello!

Running a template with doctype tags (such as <!DOCTYPE html>) appears to cause preprocessing the template into an AST to break.

Example:

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>Hi! I'm an Ember app!</title>
    <meta name="description" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel="stylesheet" id="default-css" href="/assets/my-app.css">
    {{content-for 'head'}}
    {{content-for 'head-footer'}}
  </head>
  <body>
    {{content-for 'body'}}
    {{content-for 'body-footer'}}
  </body>
</html>

run through...

// nodejs psuedocode...
const { preprocess, print } = require("@glimmer/syntax");
const path = require("path");

const fileContents = fs.readFileSync("index.html", { encoding: "utf8" });
const ast = preprocess(fileContents);
fs.writeFileSync(path.join("out", path.basename(hbsFile)), print(ast), { encoding: "utf8" });

becomes...

{{content-for "head"}}{{content-for "head-footer"}}{{content-for "body"}}{{content-for "body-footer"}}

and the AST looks like: https://gist.github.com/zimmi88/645a444f37066a58f25384713b7af822

I've tracked this to the doctype specifically because when I transform the first line into an HTML comment or remove it entirely, the output matches the input as expected.

Glimmer shouldn't usually come across doctype declarations, but supporting this might be useful for a couple of reasons... 1) Completeness. If browsers can understand it, it makes sense that Glimmer would be able to understand it too. 2) Glimmer's AST parser can reason about the index.html that ships with Ember CLI apps.

I hope this helps! Let me know if you need any additional details or information.

rwjblue commented 4 years ago

FWIW, https://github.com/tildeio/simple-html-tokenizer/pull/71 is working on fixing this.

courajs commented 2 years ago

This appears fixed by #1305. I did confirm by adding the above example into a local test, which passed.

fisker commented 1 year ago

I don't think this has been fixed.

> require('@glimmer/syntax').preprocess('<!DOCTYPE html>')
{
  type: 'Template',
  body: [],
  blockParams: [],
  loc: SourceSpan {
    data: HbsSpan {
      source: [Source],
      hbsPositions: [Object],
      kind: 'HbsPosition',
      _charPosSpan: null,
      _providedHbsLoc: [Object]
    },
    isInvisible: false
  }
}

> require('@glimmer/syntax').print(require('@glimmer/syntax').preprocess('<!DOCTYPE html><div/>'))
'<div />'
Eejit43 commented 10 months ago

Any updates on this? This is quite a pain, and causes issues in projects such as Prettier.

NullVoxPopuli commented 10 months ago

I think it's worth supporting, but personally, i'm waiting until @wycats pr here: https://github.com/glimmerjs/glimmer-vm/pull/1428/files#diff-40f60e1037245d7b8a98a7325d53890a717da9979adeb54a61a795c4ba07f9c9

Is dealt with. Too many changes.