Skip to content

/ angular-cli

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

Jump to bottom

closed link tag in index.html #16965

Open
wants to merge 1 commit into
base: master
from
Open

closed link tag in index.html #16965

wants to merge 1 commit into from
+6 −6

Conversation

@sacgrover
Copy link
Contributor

@sacgrover sacgrover commented Feb 13, 2020 •
edited

Fixes #16964
@googlebot googlebot added the cla: yes label Feb 13, 2020
@sacgrover sacgrover requested review from alan-agius4 and clydin Feb 13, 2020
@sacgrover
fix(@angular-devkit/build-angular): closed link tag in index.html
Loading status checks…
74fc504 
extractCss adds unclosed link tag, breaks XHTML index pages.

Fixes #16964
@sacgrover sacgrover force-pushed the sacgrover:unclosed-link branch from ee9cacb to 74fc504 Feb 13, 2020
@sacgrover sacgrover requested a review from IgorMinar Feb 18, 2020
@sacgrover
Copy link
Contributor Author

@sacgrover sacgrover commented Mar 20, 2020

@dgp1130 Should I close this?
@dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Mar 20, 2020

Sorry @sacgrover, looks like this PR fell through the cracks a bit.

@clydin, @alan-agius4, can one of you take a look at this? I'm thinking the fix should probably happen with how the styles are generated rather than a regex replacement afterwards. I'm not quite familiar enough to know exactly where this fix should happen though.
@IgorMinar
IgorMinar requested changes Mar 25, 2020
View changes
Copy link
Member

@IgorMinar IgorMinar left a comment

Hi @sacgrover, thanks for the pr. I think you are trying to fix a real issue but, can we please do this only as an option for xhtml deployments?

Doing this by default (for htnl) doesn't make sense, confuses people that don't know when the tags must be closed and when they don't have to be, and only bloats the payload very slightly but in the critical path.

I don't know how to implement the opt in, other cli folks should chime in on that, but it would not be too crazy to add --xhtml support to ng new and angular.json.
@dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Apr 2, 2020

@IgorMinar, I disagree on the --xhtml option. Adding another flag here includes additional complexity for developers to grok and use, for documentation to declare and explain, for the implementation to support, for testing to properly cover, and for support to give to users over time. The performance impact here is just adding a single / character for a few types of elements, so we're talking O(bytes) and it will all be compressed on top of that. I have a hard time seeing how this would have a significant enough performance impact to justify the overhead of adding and supporting a new option indefinitely.
@dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Jun 26, 2020

This PR is blocked on supporting XHTML in FW angular/angular#37779. We'll want to support it there more completely before we add support to the CLI as well, or else we'll just be whack-a-mole-ing bugs filed by users for all the edge cases missed because FW doesn't fully support it.
@bbarry bbarry mentioned this pull request Jul 23, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IgorMinar IgorMinar

@alan-agius4 alan-agius4

@clydin clydin

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
blocked cla: yes
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

`extractCss` adds unclosed `link` tag, breaks XHTML index pages
5 participants
@sacgrover @dgp1130 @IgorMinar @googlebot @alan-agius4
You can’t perform that action at this time.