Skip to content

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

Merged
mukeshpanchal27 merged 11 commits into
feature/creating-standalone-pluginsfrom
add/cli-command-for-standalone-plugins
Mar 13, 2023
Merged

CLI command for a build process to transform modules into standalone plugins#662
mukeshpanchal27 merged 11 commits into
feature/creating-standalone-pluginsfrom
add/cli-command-for-standalone-plugins

Conversation

@mukeshpanchal27
Copy link
Copy Markdown
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
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread package.json
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review March 3, 2023 11:24
@10upsimon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread package.json
Copy link
Copy Markdown
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.

Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js
Comment thread bin/plugin/commands/common.js Outdated
Comment thread bin/plugin/commands/common.js Outdated
Comment thread bin/plugin/commands/common.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Copy link
Copy Markdown
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.

Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/common.js Outdated
Comment thread bin/plugin/commands/common.js Outdated
@mukeshpanchal27
Copy link
Copy Markdown
Member Author

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

Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Copy link
Copy Markdown
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()

Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/common.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
Comment thread bin/plugin/commands/build-plugins.js Outdated
@mukeshpanchal27
Copy link
Copy Markdown
Member Author

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

Comment thread bin/plugin/commands/build-plugins.js
Copy link
Copy Markdown
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.

Comment thread bin/plugin/commands/build-plugins.js
Comment thread bin/plugin/commands/build-plugins.js Outdated
@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

3 participants