fix(schematics): add parse5 dep #11647
Conversation
googlebot
added
the
cla: yes
package.json
Outdated
| @@ -38,6 +38,7 @@ | |||
| "rxjs": "6.0.0", | |||
| "systemjs": "0.19.43", | |||
| "tsickle": "^0.27.2", | |||
| "parse5": "^5.0.0", | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Gotcha, but shouldn't we be including this anyway? Its just out of chance this is working.
There was a problem hiding this comment.
Yes, but I assumed the main goal here was to fix #11341 (which I assumed is talking about downstream apps)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@Brocco @filipesilva do you have any thoughts on how we could do HTML parsing without making parse5 a peerDep of @angular/material?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I was thinking it would make sense for the devkit to have html parsing APIs, tough obviously that would be down the road. What about using |
|
@jelbourn I feel It's easy for direct consumers to add both material and parse5. I would advise to always direct people that are updating to add parse5 though, instead of relying on hoisting. Hoisting behaviour varies a lot between package manager and is ultimately an implementation detail. |
|
@amcdnl let's go with adding an optional dependency and checking at run-time whether parse5 is available and throwing a friendly error if it isn't |
|
@jelbourn - done |
src/lib/schematics/utils/html.ts
Outdated
| @@ -10,6 +10,10 @@ import {Project} from './devkit-utils/config'; | |||
| * @param src the src path of the html file to parse | |||
| */ | |||
| export function getHeadTag(host: Tree, src: string) { | |||
| if (!parse5) { | |||
There was a problem hiding this comment.
Should this check be earlier? I want to make sure that we don't partially run the schematic before bailing out here.
jelbourn
added
pr: lgtm
action: merge
|
I just want to highlight that optional deps will always try to be installed. The installation won't fall if they fail to install, but if they don't fail to install it will stay installed. Sorry for the delay in looking at this again, have been on vacations. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds parse5 as a dependency; fixes: #11341