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

Test/hello world blueprint #908

Closed
wants to merge 14 commits into from

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Jan 3, 2024

This is a demo, it shouldn't be merged.

What is this PR doing?

Creates a helloWorld blueprint step and executes PHP code to build a response message.

What problem is it solving?

Demonstrating how to add blueprint steps.

How is the problem addressed?

By adding a new blueprint step that executes code in base-php.

Testing Instructions

  • checkout this branch
  • run npm run dev
  • Open the local site
  • See log message in dev tools PHP: Hello World!

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Jan 3, 2024

nx run docs-site:build is failing. I still need to find where to document the new step.

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Jan 3, 2024

@adamziel please let me know if you have any feedback on this.

`,
});

if (phpResponse.errors) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup! I wonder if we should just throw an exception instead. Thoughts on that? My worry is a scenario where PHP crashes without any output in stderr which could be confusing to the developer, although we could perhaps detect that based on the exit code and provide some useful guidelines in the error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would a promise reject be good here?

Copy link
Collaborator

@adamziel adamziel Jan 12, 2024

Choose a reason for hiding this comment

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

Yup! Maybe, actually. There is no error per se, the PHP program actually ran and completed, it just happened to yield a different set of output/error/exit code values. We still want to access those. Perhaps Promise.reject(response)?

/**
* Print messages to the console.
*/
helloWorld?: string;
Copy link
Collaborator

@adamziel adamziel Jan 9, 2024

Choose a reason for hiding this comment

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

For completeness – this is a new Blueprint top-level property. A step would be a part of the steps array. Either is fine, depending on what you hoped to achieve here, but I'm mentioning since you specifically used the word step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reading on, it seems like you wanted to do a step. Then this change and the one in compile.ts may be removed – You've updated the StepDefinition type so you can now use a helloWorld step as a part of the steps array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it in this way just to see if I can do it.
I will clean it up in case we want to use this as a example in the future.

@@ -33,6 +33,8 @@ import type { ValidateFunction } from 'ajv';
export type CompiledStep = (php: UniversalPHP) => Promise<void> | void;

export interface CompiledBlueprint {
/** The hello message printed in the console */
helloWorld?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

*/
export const helloWorld: StepHandler<HelloWorldStep> = async (
playground,
{ message }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! You may also want to play with the third argument (progress?), see the install-plugin.ts step for more details on that.

@adamziel
Copy link
Collaborator

adamziel commented Jan 9, 2024

This looks pretty good! You may also want to try adding a unit tests and an E2E test for that step to see how that setup works. I'm also interested in your thoughts about simplifying the class and interface structure here – it seems overly complex.

As for the build error, we have a custom Docusaurus plugin that extracts docstrings from step types. Perhaps adding an empty block comment to export type HelloWorldStep would resolve it? It would be nice to not crash in this scenario if you're up for exploring that.

@bgrgicak bgrgicak self-assigned this Jan 12, 2024
@bgrgicak
Copy link
Collaborator Author

As for the build error, we have a custom Docusaurus plugin that extracts docstrings from step types. Perhaps adding an empty block comment to export type HelloWorldStep would resolve it?

Thanks! I missed this block comment, should be good now.

It would be nice to not crash in this scenario if you're up for exploring that.

I actually think this is better. Documentation is important 🙂

@adamziel adamziel closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants