Skip to content
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(fr): clone default categories to avoid modification #13337

Merged
merged 3 commits into from
Nov 10, 2021

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Nov 8, 2021

fixes #13334

Just loading user flow code which has the FR default config in transitive deps will trigger this bug, adding a uses-responsive-images-snapshot auditRef to the perf category of the FR and the legacy configs because the category isn't copied before being altered. Since the audit only occurs in FR runs, the legacy default config then fails validation.

This PR copies the perf category before adding the auditRef.

Unclear how far we want to go with deepClone here. Just in case, should we clone settings, audits, and groups, too?

@adamraine
Copy link
Member

Just in case, should we clone settings, audits, and groups, too?

Could we just deep clone the entire legacy default config?

@brendankenny
Copy link
Member Author

Just in case, should we clone settings, audits, and groups, too?

Could we just deep clone the entire legacy default config?

Technically the default config isn't required to be compatible with a JSON roundtrip, though since the last i18n change to IcuMessage I believe what we have is compatible. This might be the simplest and we'll have to worry a lot less about other changes down the line.

Let's just do that, and we'll count on our FR config validation and tests :)

@adamraine
Copy link
Member

Technically the default config isn't required to be compatible with a JSON roundtrip

If future changes to the config make it incompatible, are we guaranteed a fatal error?

@brendankenny
Copy link
Member Author

brendankenny commented Nov 8, 2021

No, like functions are dropped as if they never existed, which mainly affects audits and gatherers definitions (which allow Classes or instances of classes). NaN becomes null, undefined becomes nothing, so it also depends on the code using it (e.g. checking for === undefined vs checking falsy).

None of it is an apparent immediate issue but it's also not something we're looking out for on the config side of things. But in case of something being dropped we may just not notice.

@paulirish paulirish added the 9.0 label Nov 8, 2021
@paulirish
Copy link
Member

@adamraine i think next action is on you here

@brendankenny
Copy link
Member Author

this should be good to go to fix the immediate bug and we can add more protections later on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants