-
Notifications
You must be signed in to change notification settings - Fork 229
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
chore(deploy-script-support): Improve coreProposalBehavior.js #8528
Conversation
// Get the manifest and its metadata. | ||
console.error('execute', { | ||
exportedGetManifest, | ||
behaviors: Object.keys(manifestNS), | ||
exportedGetManifest: manifestGetterName, | ||
behaviors: Object.keys(behaviors), | ||
}); |
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.
Questions for anyone who can answer: how important are the property names in console output, and why does this call use console.error
rather than console.info
or the log
argument?
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.
Debugging cruft that was supposed to be removed a long time ago, but which has been proven useful. I'd be happy for you to clean it up in whatever way you deem appropriate.
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.
Thanks, updated.
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.
It's a definite improvement! I have some minor notes that may help give some actionable context.
const { | ||
manifest, | ||
options: rawOptions, | ||
installations: rawInstallations, | ||
} = await manifestNS[exportedGetManifest]( | ||
} = await behaviors[manifestGetterName]( |
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'm not keen on the name behaviors
for the namespace of the manifest because behaviors[exportedGetManifest]
is not itself a "behavior". I'm not sure if this is easy to clarify in the code, or worth the time, though.
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.
Thanks for the background. I've renamed to installationNS, in alignment with similar use of evaluateBundleCap
and importBundle
elsewhere.
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 realize now that the overloading of the term "behavior" without qualifying its intent is confusing. There are two levels at which a "behavior" function is one that accepts permit-limited powers as a first argument:
- manifestBehavior (moduleBehavior?), which is specified by the manifest getter.
- coreProposalBehavior, which drives the execution of a set of manifestBehaviors.
@michaelfig Thanks for the suggestions; they all make sense to me. Back to you for probably the last round. |
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! Thanks for seeing this through.
…ture of getManifestCall
ac3daa9
to
a3a54f9
Compare
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
…salbehavior chore(deploy-script-support): Improve coreProposalBehavior.js
Description
Clearing out some local changes for the hackathon.