-
Notifications
You must be signed in to change notification settings - Fork 139
CLI command for a build process to transform modules into standalone plugins #662
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
CLI command for a build process to transform modules into standalone plugins #662
Conversation
|
@mukeshpanchal27 when functionally testing, I noticed that the plugins are not added to their own folders within build dir. therefore, while this works fine when one plugin is defined, if multiple plugins were defined it would override the PHP and other files. I'd expect built source to be placed in Instead, this is what I a seeing: |
I tested and confirmed that the last plugin in plugins.json is what will be present in the build folder in its current form. |
10upsimon
left a comment
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.
Requesting changes as per my latest comments
felixarntz
left a comment
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.
@mukeshpanchal27 Great work so far! I left a few comments for things that we can tidy up a bit. There is some duplicate code which we should avoid, and most importantly we need to remove the @since replacement as that isn't part of the requirements and doesn't work like it is here (it's not that trivial, let's ignore those for now).
felixarntz
left a comment
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.
@mukeshpanchal27 There are still a couple things to enhance here, some of that replacement logic is a bit clunky and needs further refinement.
felixarntz
left a comment
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.
@mukeshpanchal27 See my comment replies #662 (comment) and #662 (comment) for my most important feedback.
Below I pointed out just a few small details, the above comments are more important.
|
Thanks to @felixarntz and @10upsimon for the feedback. PR is ready for another round of review. |
10upsimon
left a comment
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.
@mukeshpanchal27 I have a few nitpick comments and 4 - what I believe are - important observations around handling errors/exceptions when using i/o synchronous operations like fs.readFileSync() and fs.writeFileSync()
|
Thanks @10upsimon for feedback, In 6630fef i have added exceptions for synchronous i/o operations. |
felixarntz
left a comment
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.
@mukeshpanchal27 LGTM, great work!
A few small nit-picks, but this looks good from my end.

Summary
Fixes #635
Relevant technical choices
To test this use
npm run build-pluginscommand.Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.