core(fr): add settings to context #12574
Conversation
I haven't been in any of these conversations, but at least on first read it does seem maybe too far for the sake of conceptual purity ("all artifacts come from gatherers"), but then having to special case its initialization in Is it worth re-thinking how we use the settings artifact? I think the original reason for making it an artifact was for clarity in audits. It helps clear up which settings an audit is talking about since gathering and auditing can use different settings (we never got around to allowing much difference, but we still could), and we may just not have had Since during gathering theres only one set of |
|
The more I look at this, the more I am inclined to agree. We are going to need to handle settings differently in any situation. Creating a pseudo-gatherer to house settings just so it can be used as a dependency is overly convoluted. Other base artifacts do make sense as normal gatherers, but I also don't like the idea of adding a |
|
I'm also completely fine with The important thing with settings, as @brendankenny already noted, is that the dependency on the auditing side is reflected, as the settings at gather mode time vs. audit time will in fact make a difference in some contexts. |
| @@ -63,8 +63,8 @@ describe('Fraggle Rock Config', () => { | |||
| }); | |||
|
|
|||
| it('should throw on invalid artifact definitions', () => { | |||
| const configJson = {artifacts: [{id: 'FullPageScreenshot', gatherer: 'full-page-screenshot'}]}; | |||
| expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/FullPageScreenshot gatherer/); | |||
| const configJson = {artifacts: [{id: 'ScriptElements', gatherer: 'script-elements'}]}; | |||
There was a problem hiding this comment.
We are running out of candidates for this hehe
adamraine
changed the title
| @@ -63,8 +63,8 @@ describe('Fraggle Rock Config', () => { | |||
| }); | |||
|
|
|||
| it('should throw on invalid artifact definitions', () => { | |||
| const configJson = {artifacts: [{id: 'FullPageScreenshot', gatherer: 'full-page-screenshot'}]}; | |||
| expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/FullPageScreenshot gatherer/); | |||
| const configJson = {artifacts: [{id: 'ScriptElements', gatherer: 'script-elements'}]}; | |||
Opening as a draft because I'm not sold on the way I made settings into a dependency.
Some other options I considered:
settingsdirectly toFRTransitionalContextsettingsmanually incollectArtifactDependenciesI converted
FullPageScreenshotas an example of how this would be used.