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
Feat/add config hook #864
Feat/add config hook #864
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/3mra2hcje |
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 great! Left some comments
let needWarnAfterCreated = false; | ||
let warned = false; | ||
let plugin: SnowpackPlugin | null = null; | ||
let name: string; |
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 think there's an ordering problem here, where "name" may not yet be defined. Can execPluginFactory
take an additional name argument, to make it explicit for the logger?
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.
We maybe not know the name of plugin when we call the execPluginFactory function. So the name is assigned by name = plugin.name || pluginFactory.name
.
The assign of name
is executed before the executing of logWarn
.
There is no case that name
is undefined through running the yarn test
.
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.
The ordering issue looks like it's at:
plugin = pluginFactory(configProxy, pluginOptions) as SnowpackPlugin;
// This triggers the proxy, which triggers logWarn, which references `name` before it's set
name = plugin.name || pluginFactory.name;
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.
Because the name of plugin is known after the pluginFactory
returns, I delay the warn until the return of pluginFactory
if plugin references snowpackConfig while generating plugin object.
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.
The ordering issue looks like it's at:
plugin = pluginFactory(configProxy, pluginOptions) as SnowpackPlugin; // This triggers the proxy, which triggers logWarn, which references `name` before it's set name = plugin.name || pluginFactory.name;
It's not happen. The assign of name
is synchronously executed after the pluginFactory
executes and returns.
Edit: if executing pluginFactory
triggers the proxy, I set a flag needWarnAfterCreated = true
and execute logWarn after assign of name
.
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.
LGTM, I may move a few things around before release but overall this is great! Thanks for tackling!
Changes
do the stage 1 and stage 2 of #621
Testing