Skip to content

Conversation

@mukeshpanchal27
Copy link
Member

Summary

Fixes #635

Relevant technical choices

To test this use npm run build-plugins command.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Mar 2, 2023
@mukeshpanchal27 mukeshpanchal27 self-assigned this Mar 2, 2023
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review March 3, 2023 11:24
@10upsimon
Copy link
Contributor

@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 builds/[slug]/ where [slug] is the plugins slug as defined in the plugins.json.

Instead, this is what I a seeing:

image

@10upsimon
Copy link
Contributor

@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 builds/[slug]/ where [slug] is the plugins slug as defined in the plugins.json.

Instead, this is what I a seeing:

image

I tested and confirmed that the last plugin in plugins.json is what will be present in the build folder in its current form.

Copy link
Contributor

@10upsimon 10upsimon left a 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

Copy link
Member

@felixarntz felixarntz left a 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).

Copy link
Member

@felixarntz felixarntz left a 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.

Copy link
Member

@felixarntz felixarntz left a 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.

@mukeshpanchal27
Copy link
Member Author

Thanks to @felixarntz and @10upsimon for the feedback. PR is ready for another round of review.

Copy link
Contributor

@10upsimon 10upsimon left a 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()

@mukeshpanchal27
Copy link
Member Author

Thanks @10upsimon for feedback, In 6630fef i have added exceptions for synchronous i/o operations.

Copy link
Member

@felixarntz felixarntz left a 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.

@mukeshpanchal27 mukeshpanchal27 merged commit 9806fb5 into feature/creating-standalone-plugins Mar 13, 2023
@mukeshpanchal27 mukeshpanchal27 deleted the add/cli-command-for-standalone-plugins branch March 13, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement CLI command to for a build process to transform modules into standalone plugins

4 participants