-
Notifications
You must be signed in to change notification settings - Fork 58
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: allow build event handlers to be passed programatically to the build execution #5362
feat: allow build event handlers to be passed programatically to the build execution #5362
Conversation
This pull request adds or modifies JavaScript ( |
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.
Left a couple of non-blocking comments, but generally looks good to me!
] | ||
const HIDDEN_FLAGS = [...SECURE_FLAGS, ...TEST_FLAGS, ...INTERNAL_FLAGS] | ||
const HIDDEN_DEBUG_FLAGS = [...SECURE_FLAGS, ...TEST_FLAGS] | ||
const HIDDEN_DEBUG_FLAGS = [...SECURE_FLAGS, ...TEST_FLAGS, 'eventHandlers'] |
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.
Any reason to hide this from the debug view? I think it could be useful when troubleshooting.
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.
So this was due to the functions not being able to be parsed by the YAML method underneath. Didn't dig too deep, but it was something in the visualisation it wasn't happy with it being a function.
Good shout on it being useful, may show the entries + description here
let handler = eventHandler | ||
|
||
if (typeof eventHandler !== 'function') { | ||
description = eventHandler.description |
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 this means that if you specify a handler
property but no description
, we'll end up with no description. Feels like we could use the default description in that case?
Example
await build({
eventHandlers: {
onPostBuild: {
handler: () => {
console.log('on post build');
}
},
},
});
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.
Yeah! Good catch
const stepsA = addCoreSteps(steps) | ||
const stepsB = sortSteps(stepsA, EVENTS) | ||
const eventSteps = getEventSteps(eventHandlers) |
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.
nit: I think eventHandlerSteps
would be a bit more descriptive
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.
💯
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 good to me!
🎉 Thanks for submitting a pull request! 🎉
Summary
Part of CT-151
Adds a
eventHandlers
property, which allows definition of the build event handlers (onBuild
,onPostBuild etc.
) to be passed as custom steps into the build execution.This is to help enable parity of deploys within the CLI, where we need to change the deploy logic to run within the
onPostBuild
stage much like in netlify hosted builds, rather than after the full build execution.Went with a more generic route here to keep our options open in the future for doing things within the build process.
or, to supply a custom description
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)