autorender removes `<link>` tags #41

justinbmeyer · 2017-01-12T02:02:05Z

Currently, autorender removes <script>, <link>, and <style> tags that are in the template. This causes bugs when these tags are expected to be part of the page, for example a favicon:

<link rel="icon" type="image/png" href="/favicons/favicon-32x32.png" sizes="32x32">

Why are these removed? What can be done to allow these types of tags? Thanks!

cc @lkodai

The text was updated successfully, but these errors were encountered:

justinbmeyer · 2017-01-12T02:15:33Z

Since creating this issue, I think I understand why it does this -> to prevent a double load of script, link, and style tags if the app has been server-side rendered (SSR).

The app using done-autorender is not using SSR, but it has things like <link rel="icon" type="image/png" href="/favicons/favicon-32x32.png" sizes="32x32"> in its "head".

I see a few options:

1. Have this project only use `<body>` in index.stache

Presumably, much of what's in index.stache's <head> should be in the page that is being sent back by the server. If people are using done-autorender in non SSR environments, we should probably encourage people to do this.

We could:

Detect if we aren't in an SSR environment and if there are script, head, or style tags in the head, we tell people to use the <body>-only version of done-autorender.
- How can we detect if we are SSR? Special attribute on the SSR-ed HTML?
Detect when there is only a <body> or a lack of <body>.

2. Support both served-`<head>` and autorendered-`<head>` elements

This could be tricky as the elements might not be the same. We could do a diff and only add what's different. But it feels strange to have a different behavior of the <body> vs the <head>.

3. Detect we aren't doing SSR and do a full replacement of head.

If done-autorender knows it's a pure client-render situation, it should not ignore anything in the head.

My thoughts

We should do #1 + #3.

matthewp · 2017-01-12T13:22:56Z

You can put links in the body too, so #1 doesn't solve anything. I think we can do #2. We're only talking about links with an href and scripts here so not a big deal to keep a map of the ones already in the page.

justinbmeyer · 2017-01-12T14:26:07Z

Why not start with #3 as it's probably the easiest? I still think we should do #1, but just warn people they should probably put it in the served HTML for better performance.

…

Sent from my iPhone

On Jan 12, 2017, at 7:22 AM, Matthew Phillips ***@***.***> wrote: You can put links in the body too, so #1 doesn't solve anything. I think we can do #2. We're only talking about links with an href and scripts here so not a big deal to keep a map of the ones already in the page. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

matthewp · 2017-01-12T14:44:49Z

Limiting done-autorender to only the <body> would remove some valid use-cases such as dynamically setting the title:

<head>
  <title>My site | {{page}}</title>
</head>
<body>
 ...
</body>

As far as #3 goes, how would you know that you're in a client-only rendering situation? The only thing I can think is to add some type of attribute to the markup. Maybe something like this:

<html autorender-all>
  ...
</html>

justinbmeyer · 2017-01-12T14:50:45Z

Sorry, to be clear, I'm not suggesting the body-only part of #1, I'm suggesting simply having a warning to tell people they probably should be putting these things in their served page.

Yes for #3 that's exactly what I suggested above:

How can we detect if we are SSR? Special attribute on the SSR-ed HTML?

It would probably be ideal if this was a className ... mostly because it would be easy to style the ssr-ed HTML in a disabled mode.

matthewp · 2017-01-12T14:58:52Z

In a disabled mode? Can you clarify what you mean there?

justinbmeyer · 2017-01-12T15:01:00Z

I mean the style sheet can make the site look a shade of gray before re-attachment, to give the user a clear signal that the site is un-usable.

.done-ssred {
  color: #808080
}

If we remove that className after re-attachment, then the text color will go to black.

matthewp · 2017-01-12T16:15:07Z

That makes omitting duplicate links/scripts an opt-in rather than opt-out feature. I don't think it should be opt-in. I don't think anyone ever wants duplicates so they shouldn't have to add a class to prevent them.

matthewp · 2017-01-12T21:56:47Z

After talking about it with @justinbmeyer sort of concluded that there is no good reason to put scripts or links into both the HTML and the Stache files. You would put them in one or the other, in which case there wouldn't be duplicates.

The only scenario for putting it in both is when server rendered. So we're going to have done-autorender add an attribute when being server rendered to the documentElement that gets removed after reattachment.

Some ideas for the attribute:

unresolved
unattached
detached
not-live
reattaching

I kind of like unattached or detached.

matthewp · 2017-01-13T13:19:17Z

Need tests for:

- server rendered
- The attribute should be added.
- Reattaching - SSR
- The attribute should be removed.
- No duplicates should exist.
- Reattaching - No SSR
- all links/scripts should be copied over.

This implements a new attribute that is added to the documentElement, *data-detached*. it is added in SSR and removed when attachment is complete on the client. Because of this new attribute, we will insert any links/styles/scripts when that attribute doesn't exist. Fixes #41

matthewp · 2017-01-18T13:32:16Z

This is in 1.0.0-alpha.3, here's how to use it: https://github.com/donejs/autorender/tree/major#server-rendering

matthewp · 2017-01-19T18:11:38Z

The existing solution has a bug where upon rerendering (for live-reload), because the data-detached attribute is gone it will insert scripts even though they are already there. This causes a problem where the Steal script tag is continuously reinserted and the entire app loads over and over and over.

I've reverted the feature since it's such a bad bug temporarily while I fix it.

This is related to the data-detach feature. During live reload we go through reattachment again, in which case script/link/style elements are on the page. We need to make sure not to insert them again. Closes #41

matthewp · 2017-01-19T21:38:40Z

I reverted this again. This is really hard to get right 😕 You have to maintain state to know if reattachment has occurred and if so, use the old behavior of not push in scripts. Otherwise if you push in a steal script you'll get cycles as it continuously reinserts steal over and over again.

This feels a little dangerous to me and I'd like to back away from it and rethink it through a bit.

matthewp · 2017-02-13T21:17:46Z

@chasenlehara why did you put this on the DoneJS 1.0 release? I don't think it should be. As I detailed about, this is really easy to get wrong and getting it wrong is very bad consequences (causing the browser to lock up, for example).

And it serves a pretty niche need (putting links in index.stache but not in your html).

chasenlehara · 2017-02-13T21:58:04Z

I put it in the 1.0 release epic because it’s a p1, but we can move it to another release epic or out completely if you don’t think we should address it now or schedule it for a future release. 🙂

matthewp · 2017-02-13T22:02:52Z

I don't think this should be part of the 1.0 release or that it's a P1.

matthewp · 2017-02-13T22:08:35Z

To clarify, I've fixed this twice only to find that it caused even worse bugs. Specifically when coupled with live-reload it would cause steal.js to be inserted multiple times which caused infinite recursion (each time steal.js was inserted it would then re-import your entire app including done-autorender which would then insert steal.js which would re-import your entire app, etc).

This can be fixed but given it is highly stately thing I'd want to refactor all of done-autorender to make sure that this feature could be isolated and properly tested for all scenarios. done-autorender's code is already not the best, so I wouldn't want to shove this feature in in a way that isn't readable.

pYr0x · 2017-09-13T22:40:31Z

@matthewp is this issue solved with the current 1.3 release?

matthewp · 2017-09-14T01:19:03Z

Nope

pYr0x · 2017-09-14T09:03:07Z

so what is the current workaround?

matthewp · 2017-09-14T12:14:04Z

Put the link tags into your stache file, I think.

pYr0x · 2017-09-20T07:41:36Z

as justin said #41 (comment) , link tags get removed ... so putting them into the stache file dont work.
or do i something missunderstand?

matthewp · 2018-02-07T21:22:40Z

In 2.0 you'll be able to keep elements in the DOM using a special data attribute. This is explained in the readme:

https://github.com/donejs/autorender/tree/major#keeping-elements-in-the-dom

justinbmeyer added the bug label Jan 12, 2017

matthewp self-assigned this Jan 12, 2017

matthewp added the p1 label Jan 12, 2017

matthewp mentioned this issue Jan 17, 2017

Implements data-detached #43

Merged

matthewp mentioned this issue Jan 19, 2017

When reattaching, only do a full replacement once #45

Merged

matthewp closed this as completed Jan 19, 2017

matthewp reopened this Jan 19, 2017

matthewp removed their assignment Mar 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autorender removes `<link>` tags #41

autorender removes `<link>` tags #41

justinbmeyer commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017 •

edited

Loading

matthewp commented Jan 12, 2017 •

edited

Loading

justinbmeyer commented Jan 12, 2017 via email

matthewp commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017

matthewp commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017

matthewp commented Jan 12, 2017

matthewp commented Jan 12, 2017 •

edited

Loading

matthewp commented Jan 13, 2017 •

edited

Loading

matthewp commented Jan 18, 2017

matthewp commented Jan 19, 2017 •

edited

Loading

matthewp commented Jan 19, 2017

matthewp commented Feb 13, 2017 •

edited

Loading

chasenlehara commented Feb 13, 2017

matthewp commented Feb 13, 2017

matthewp commented Feb 13, 2017

pYr0x commented Sep 13, 2017

matthewp commented Sep 14, 2017

pYr0x commented Sep 14, 2017

matthewp commented Sep 14, 2017

pYr0x commented Sep 20, 2017

matthewp commented Feb 7, 2018

autorender removes <link> tags #41

autorender removes <link> tags #41

Comments

justinbmeyer commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017 • edited Loading

1. Have this project only use <body> in index.stache

2. Support both served-<head> and autorendered-<head> elements

3. Detect we aren't doing SSR and do a full replacement of head.

My thoughts

matthewp commented Jan 12, 2017 • edited Loading

justinbmeyer commented Jan 12, 2017 via email

matthewp commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017

matthewp commented Jan 12, 2017

justinbmeyer commented Jan 12, 2017

matthewp commented Jan 12, 2017

matthewp commented Jan 12, 2017 • edited Loading

matthewp commented Jan 13, 2017 • edited Loading

matthewp commented Jan 18, 2017

matthewp commented Jan 19, 2017 • edited Loading

matthewp commented Jan 19, 2017

matthewp commented Feb 13, 2017 • edited Loading

chasenlehara commented Feb 13, 2017

matthewp commented Feb 13, 2017

matthewp commented Feb 13, 2017

pYr0x commented Sep 13, 2017

matthewp commented Sep 14, 2017

pYr0x commented Sep 14, 2017

matthewp commented Sep 14, 2017

pYr0x commented Sep 20, 2017

matthewp commented Feb 7, 2018

autorender removes `<link>` tags #41

autorender removes `<link>` tags #41

justinbmeyer commented Jan 12, 2017 •

edited

Loading

1. Have this project only use `<body>` in index.stache

2. Support both served-`<head>` and autorendered-`<head>` elements

matthewp commented Jan 12, 2017 •

edited

Loading

matthewp commented Jan 12, 2017 •

edited

Loading

matthewp commented Jan 13, 2017 •

edited

Loading

matthewp commented Jan 19, 2017 •

edited

Loading

matthewp commented Feb 13, 2017 •

edited

Loading