-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add the source map for Angular TodoMVC build. #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I'd like @lpardosixtosMs to have a look so that he can give some guidance to update the complex version.
Also I'd like Bas to check there's no issue with the Base as we had in the past (it seems to be it's OK but I'm not confident).
@@ -92,5 +92,8 @@ | |||
} | |||
} | |||
} | |||
}, | |||
"cli": { | |||
"analytics": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for turning this off!
@@ -10,7 +10,7 @@ | |||
"scripts": { | |||
"ng": "ng", | |||
"dev": "ng serve", | |||
"build": "ng build --base-href /resources/todomvc/architecture-examples/angular/dist/", | |||
"build": "ng build --source-map=true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep the base-href
in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what this does? It's not clear to me. If all it does is adding the <Base>
to the html file, then maybe we don't need it after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Angular needs to be able to find the files when running inside Speedometer, but @rniwa also pointed out before that there was an issue when using <base>
in the html file when running on the webkit infra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least this works without it currently (I made sure to try it locally), so it's probably not needed unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting rid of --base-href
gets rid of the base element in the build so I don't think we want to keep that. As far as I can tell, there is no other side effect from this change.
It should be enough to rebuild the complex version, the script will copy the source maps too, @issackjohn is verifying locally. We usually check that the is not in the html too but looks like it is not generated anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.