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: internalize normalized configs #14547

Closed
wants to merge 2 commits into from

Conversation

adamraine
Copy link
Member

#14048

No longer exposing generateConfig and generateLegacyConfig on the surface api.

Some renames to make this more clear

  • Config -> LegacyNormalizedConfig
  • FRConfig -> NormalizedConfig

@adamraine adamraine requested a review from a team as a code owner November 18, 2022 17:32
@adamraine adamraine requested review from connorjclark and removed request for a team November 18, 2022 17:32
}
}
// @ts-expect-error Breaking the NormalizedConfig type.
jsonConfig.navigations = undefined;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: we don't want users to know about this

@brendankenny
Copy link
Member

brendankenny commented Nov 18, 2022

It might be worth discussing the goals with generateConfig and generateLegacyConfig in the next sync. One key point is that configs aren't internal structures, they're just the normalized version of an input "config json" with all the optional properties filled in. Configs are required to be valid config jsons that normalize to themselves.

generateConfig doesn't expose anything that should be shielded from the user because it just returns a config, which users can already make themselves, and any breaking change to that format would be a public-facing breaking change anyways (navigations is the ugly spot on that, but that's why we already plan to include it in the 11.0 clean-up breaking changes).

If the reason to get rid of them isn't for hiding internals but just for keeping the API simple (why would a user need to call this?), I can see that but it also seems like it might be better postponed for the separation of the core interface from the node interface in #14048. It might not make sense for node users to call generateConfig (though is #14048 (comment) resolved?), but in terms of architecture layering, cli/bin.js should ideally only interact with the public core/index.js API, not reach inside. The client entry points have also drifted from only interacting with core/index.js as they did at one point, but as #14048 progresses hopefully we can get back there.

@brendankenny
Copy link
Member

re config naming:

input-config format vs normalized-config format is the same divide that the Config.Json vs Config names were added to differentiate. At the time we recognized that it wasn't all that self explanatory (notably the config json can actually be js :), but almost all cases where code touches a config (or part of a config) it's actually a normalized config, so we preferred to give that one the short and sweet name Config. We completely failed on coming up with a compelling name for the pre-normalization format, but at least it was just a little code in cli/ and config.js that had to live with configJson/Config.Json.

That decision still seems ok. NormalizedConfig is a mouthful when capital-C Config conveys the same thing. If we're primarily concerned with differentiating the public API, revisiting the "config json" bikeshed seems like it could yield a bigger improvement.

@adamraine
Copy link
Member Author

generateConfig doesn't expose anything that should be shielded from the user because it just returns a config, which users can already make themselves, and any breaking change to that format would be a public-facing breaking change anyways (navigations is the ugly spot on that, but that's why we already plan to include it in the 11.0 clean-up breaking changes).

navigations is indeed the ugly spot that motivated this. With the new FR config as default, anyone already using generateConfig will find themselves working with this weird internal property on an otherwise normalized object. Perhaps we could throw a delete navigations in the generateConfig to prevent that 🤷

though is #14048 (comment) resolved?

Yeah, sound like they were using generateConfig because of a bug with skipAudits and onlyAudits together. Is this correct @paulirish @benschwarz?

That decision still seems ok. NormalizedConfig is a mouthful when capital-C Config conveys the same thing. If we're primarily concerned with differentiating the public API, revisiting the "config json" bikeshed seems like it could yield a bigger improvement.

I prefer the more specific name on our internal stuff. If we are moving to publicly exposed types, it would be better to have the Config type reserved for the config objects users will be interacting with. That's putting the cart before the horse though so I won't die on this hill.

@adamraine
Copy link
Member Author

Discussed with @paulirish and @brendankenny. Path forward looks like:

  • Remove the generateConfig functions
  • Remove the --print-config CLI flag
  • Config.FRConfig -> ResolvedConfig and make all variables of this type resolvedConfig
  • Config.Config -> LegacyResolvedConfig and make all variables of this type resolvedConfig
  • Config.Json -> Config and make all variables of this type config

@adamraine
Copy link
Member Author

Closing and splitting this up

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

Successfully merging this pull request may close these issues.

4 participants