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

Attributes are missing the quotes from version 4.21.0 #1602

Closed
tornord opened this issue Sep 8, 2020 · 10 comments
Closed

Attributes are missing the quotes from version 4.21.0 #1602

tornord opened this issue Sep 8, 2020 · 10 comments
Labels
type:unverified bug A bug report that has not been verified

Comments

@tornord
Copy link

tornord commented Sep 8, 2020

Marko Version

=4.21.0

Details

From version 4.21.0 til latest 4.23.2, quotes on attributes have changed behavior. Before 4.21.0, attributes get double quotes by default. After 4.21.0 attributes are missing the quotes.

Expected Behavior

<a href="path">Hello</a>

Actual Behavior

<a href=path>Hello</a>

Possible Fix

Function guessQuotes in marko/packages/marko/src/runtime/html/helpers/attr.js has changed. In version <=4.20.2 guessQuotes normally returns doubleQuote, in later versions it doesn't.

Your Environment

  • Environment name and version (e.g. Chrome 39, node.js 5.4): Node.js both v10.21.0 and v12.18.3
  • Operating System and version (desktop or mobile): MacOS 10.15.6
  • Link to your project: N/A

Steps to Reproduce

  1. Create an empty folder.
  2. Install
  1. Create the file hello.marko with the following content:
<a href="path">Hello</a>
  1. Create the file main.js with the following content:
require("marko/node-require");
const hello = require("./hello.marko");
console.log(hello.renderToString({}));
  1. Run:
node main.js

Should output:

<a href=path>Hello</a>

Stack Trace

N/A

@tornord tornord added the type:unverified bug A bug report that has not been verified label Sep 8, 2020
@tigt
Copy link
Contributor

tigt commented Sep 8, 2020

This was a feature to decrease HTML output size (#1432). Is it causing problems for you?

@tornord
Copy link
Author

tornord commented Sep 9, 2020

Hello!

Yes, when we use these newer versions of Marko.js together with a library called Local-ESI for simulating Akamai ESI (https://www.npmjs.com/package/@bonniernews/local-esi) in our tests, the parsing of content returns unexpected results. We get missing first slashes "/some-path/" becomes "some-path/" and certain text within HTML tags end up in the wrong place.

This could be just Local-ESI doing things wrong, but we can't really test this in production with the real Akamai.

Removing single / double quotes is no small thing considering how many potential different parsers there are out there. We suggest an option for preserving double quotes where they were. An option like writeToDisk. What do you think about that? Maybe we could try to add it ourselves with a PR to you?

@tigt
Copy link
Contributor

tigt commented Sep 9, 2020

I’m only a contributor and not a Marko maintainer, so I can’t speak to how much they like that idea. But I suppose it can’t hurt to try.

If this doesn’t move fast enough for you though, it may also be worth trying to PR/fix that buggy quoteless value parsing in @bonniernews/local-esi or its underlying HTML parser, atlas-html-stream.

@tornord
Copy link
Author

tornord commented Sep 11, 2020

We can probably fix the issue with the esi-tags, but the safest way for us would be to continue with Marko rendering attributes values with quotes, as Marko did until 4.21. Changing the output format is risky, it can cause problem in other systems as well.
Adding an option "render attribute values with quotes" in Marko would be the best. @tigt , do you know who we can discuss this with? We can create a PR.

@tigt
Copy link
Contributor

tigt commented Sep 11, 2020

I believe the core team members are now @DylanPiercey, @mlrawlings, and @ryansolid

@tornord
Copy link
Author

tornord commented Sep 14, 2020

Thanks @tigt!

@tigt
Copy link
Contributor

tigt commented Oct 19, 2020

@tornord, is this still giving you problems? If you want to talk it over in a more immediate format, the Marko Discord is pretty active, especially after 1pm California time.

@williamvilasboas
Copy link

Hi, I'm having the same problem: '(

@dalexanco
Copy link

Hi there, this is a problem for us too... I would suggest an option (default=true) to define if marko shoud optimize quotes rendering.

@DylanPiercey
Copy link
Contributor

Currently, Marko only targets spec-compliant HTML parsers and aims to produce the smallest HTML output given that.

I don't think we plan to change direction to output larger HTML for non-compliant parsers and tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:unverified bug A bug report that has not been verified
Projects
None yet
Development

No branches or pull requests

5 participants