Skip to content

fix(schematics): add parse5 dep #11647

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 28, 2018
Merged

fix(schematics): add parse5 dep #11647

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ab3a81
fix(schematics): add parse5 dep
amcdnl Jun 4, 2018
237d84f
Merge remote-tracking branch 'origin/master' into parse5
amcdnl Jun 15, 2018
05fde46
chore: pr feedback
amcdnl Jun 15, 2018
d0b9120
chore: nit
amcdnl Jun 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"rxjs": "6.0.0",
"systemjs": "0.19.43",
"tsickle": "^0.27.2",
"parse5": "^5.0.0",
Copy link
Member

@jelbourn jelbourn Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't solve the problem since this doesn't do anything for downstream apps. For people using schematics, they would need to have parse5 installed in their app. However, we don't want to include that as a real dependency in the distributed package.json. Maybe that's motivation to use something else?

Copy link
Contributor Author

@amcdnl amcdnl Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, but shouldn't we be including this anyway? Its just out of chance this is working.

Copy link
Member

@jelbourn jelbourn Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I assumed the main goal here was to fix #11341 (which I assumed is talking about downstream apps)

Copy link
Contributor Author

@amcdnl amcdnl Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, that was the original goal but as you said it won't solve the problem. Let me know how you would like to proceed.

Copy link
Member

@jelbourn jelbourn Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Brocco @filipesilva do you have any thoughts on how we could do HTML parsing without making parse5 a peerDep of @angular/material?

Copy link
Contributor

@filipesilva filipesilva Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not really sure. If this package uses parse5 then it should be a dependency. But I see the problem in adding when it's just used for the update schematics.

I suppose one solution is to separate the Material schematics into a separate package. Another is to provide html parsing facilities within schematics. But one way or another, Material would have parse5 somewhere in its dependency (or peer dependency) tree.

Copy link

@Brocco Brocco Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The little bit of HTML updating we've done is via regex, but we're also only adding some tags inside of the head tag for PWA.

"tslib": "^1.9.0",
"zone.js": "^0.8.26"
},
Expand Down