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
Load global config #9021
base: v2
Are you sure you want to change the base?
Load global config #9021
Conversation
if (globalConfig) { | ||
// I believe this should be done per file? | ||
for (let devDep of globalConfig.devDeps) { | ||
await this.addDevDependency(devDep); |
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 assumed I should be adding dev deps for all files using the global config even though it's already loaded. Just confirming this is correct?
// inlineEnvironment: true | ||
// All files can inject all ENV vars | ||
assignAllEnv(publicEnv); | ||
} else if (inlineEnvironment === 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.
While I think the existing inlineEnvironment
behaviour is a bit confusing, this is attempting to replicate it exactly.
Specifically, I'm not a huge fan of default behaviour being different to all the possible config options. We can discuss the actual behaviour separately though.
plugin: transformer.name, | ||
searchPath: toProjectPathUnsafe('index'), | ||
// Project root should be considered source? | ||
isSource: 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.
Wasn't sure about this one?
@@ -205,6 +212,7 @@ export default class Transformation { | |||
let configRequests = getConfigRequests([ | |||
...this.configs.values(), | |||
...this.resolverRunner.configs.values(), | |||
...this.globalConfigs.values(), |
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.
Just confirming this is correct?
↪️ Pull Request
This PR implements a new transformer hook called
loadGlobalConfig
that only runs once per build (rather than once per file). This is useful when you have a transform with project level configuration and only want to calculate it once.I have also updated the JS and CSS transformers to make use of the hook. The JS transformer is most impacted with the environment variable inline maps being calculated only once per build now.
I had a few questions around implementation that I will leave as line specific comment below.
💻 Examples
🚨 Test instructions
I haven't added any tests yet as I wasn't sure how to approach testing this. However the existing tests for the JS transform should cover the JS specific functionality. I have also manually validated that the hook is being called when expected during re-builds.
✔️ PR Todo