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

DOCUMENT provider refactoring #238

Closed
alexpods opened this issue Jan 31, 2016 · 11 comments
Closed

DOCUMENT provider refactoring #238

alexpods opened this issue Jan 31, 2016 · 11 comments
Assignees

Comments

@alexpods
Copy link

Currently provider for DOCUMENT requires appComponentType (see it here). It looks very strange to me, because we provide appComponentType only on bootstrapping phase, not when we're creating an application. Also this provider creates mock document which makes it very difficult to implement services for changing document title, meta tags and so on. I think it would be much better if we created a real document there based on a real html.

I've implemented this in up branch of angular2-universal-starter.

The main points:

  • Every application should be bound with a particular DOCUMENT_HTML (see it here and here)
  • Provider for DOCUMENT creates a real document based on this DOCUMENT_HTML (see it here)
  • parse5 serialization is used after bootstrapping (see it here).
  • My benchmark shows the same speed as an original implementation.

Pros

  • No need to provide appComponentType as an application provider.
  • Now implementing services like Title becomes a trivial task. For example see ServerTitle implementation here. In this case if we have some workers page like:
@Component({
  selector: 'worker',
  template: '<h1>Workers Page</h1>'
})
export class Workers {
  constructor(titleService: Title) {
    titleService.setTitle('Workers page!!!');
  }
}

then prerendered html will contain <title> tag with "Workers Page!!" text. And I want to mention that <title> tag is very important for search engines.

What do you think about it?

@PatrickJS
Copy link
Member

We probably should add you to the repo since you understand what's going on. We need to support both ways of creating the document but your implementation is solid. We should probably megre your changes. There are a couple of things we need to sync up on

@PatrickJS PatrickJS self-assigned this Jan 31, 2016
@jeffwhelpley
Copy link
Contributor

+1 on adding @alexpods with full access to repo. The only thing I wonder about with this particular issue is whether there will be any perf hit for using the parse 5 HTML object as opposed to our own implementation. I do think we should go with Alex's suggestion regardless to see what happens.

Btw, this touches on the meta tag service that is in another issue. We should all brainstorm on how we want this to work because it is going to come up with many people that use Universal. My initial thought is that perhaps we should have a metaTagService/seoService/headService/whatever that is part of Angular Universal which is used to set title, keywords, facebook title, rel href tags, etc., etc.

In addition to the Universal service, we should have (ideally) a declarative way of specifying these values. IMO, meta tags change depending on 2 factors: route and current state. As such, perhaps we have something that is part of the RouteConfig decorator and/or some sort of state decorator (I can show you some stuff I am doing on this in my current app).

@PatrickJS
Copy link
Member

@alexpods can you open a pull-request?

@alexpods
Copy link
Author

alexpods commented Feb 2, 2016

@gdi2290 Yes, sure. Right now I'm busy, but later today PR will be done (right now 11:30 AM in Moscow).

@jeffwhelpley My benchmark shows that there's no performance regression (at least with a small app). But we definitely should optimize performance as much as we can. And we should start with reusing the same services for different requests. For example reusing already created ProtoViews can give us big boost of performance. Maybe we should move these services to NODE_PLATFORM_PROVIDERS?

@jongood01
Copy link

+1 - Yes please do this. Need to be able to have pre-rendered dynamic title and meta tags

@Settler
Copy link

Settler commented Feb 17, 2016

+1 for headService.
Head tags is very important in SEO and links sharing via messengers. For SEO it is Title tag. For links sharing it is open-graph protocol tags (http://ogp.me/) that supported by Skype, Telegram, etc. Title is important on both sides - server and client. Other tags is important only for server side. It would be nice, if universal will have at least abstract server side headService, that supports manipulation of tags in head. Then any programmer could implement it own service that will support any specific set of tags. For example base headService could provide simple dom manipulation methods like set(), setAttribute(), setValue(), get(), getAttribute(), getValue(). And custom openGraph service could use headService, but have strict api like: setTitle(), setDescription(), setImageUrl() etc. And implementation of that specific service will be a trivial task, if I have headService.

@samvloeberghs
Copy link

+1 - yes please

@PatrickJS
Copy link
Member

closing this as everything has been merged other than head service

@justme1
Copy link

justme1 commented Jun 9, 2016

any news regarding the head service?

@jeffwhelpley
Copy link
Contributor

@justme1 it is on our ToDo list, but right behind two other high priority items that we need to get done first. If you or anyone else from the community wants to help out on this, let me know.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants