-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core: add source-maps gatherer to default config #10990
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.
🎉 🎉
unused-javascript
should move 'SourceMaps'
out of __internalOptionalArtifacts
too?
And is there a simple source map we could add to dbw-tester to not change it too much but also get them to start showing up in sample_v2.json
?
@@ -230,7 +223,7 @@ describe('Config', () => { | |||
'ViewportDimensions', // from gatherer | |||
], | |||
__internalOptionalArtifacts: [ | |||
'SourceMaps', // Not in the config. | |||
'SourceMapsBlah', // Not in the config. |
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.
looks like this is exactly the same test as on line 151?
somehow these got repeated three times :)
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.
somehow these got repeated three times :)
does not throw when an audit requests an optional artifact with no gatherer supplying it
is still in there one extra time and should keep optional artifacts in gatherers after filter
test is repeated three times :)
We have plenty of other end-to-end tests for source maps (smoke, legacy-js), so this isn't too critical. It seems like the entire sample artifacts could use a reset, seems better than patching as we go. |
I was suggesting actually adding them to dbw-tester, not an artifacts patch. Anyways, it's not critical, it was just a suggestion based on #10978 (comment), which, let's just check who left thumbs up reactions on that comment... :P |
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.
nice to delete the repeated tests, otherwise LGTM!
No description provided.