-
Notifications
You must be signed in to change notification settings - Fork 0
v1 continuous integration #4
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
Conversation
|
@ALJ i'm just gonna merge this to move us along but would love any input you have on this. |
| /node_modules | ||
| /vendor | ||
|
|
||
| vendor/**/* |
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.
@braican similar question to my gitignore/build one... why are inner /vendor folders gitignored, but not autoload, or composer/?
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.
Prob nevermind this Q, too as there's info in the PR description
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.
The reasoning here is similar to that of the static assets, with some additional context around the nuances of using Composer with a WordPress project.
For some setup - this plugin uses PHP namespacing and includes an autoloader that is built by Composer, which gets added to the vendor/ directory when composer install is run when working on this package.
This is where things get interesting - if this was purely a PHP package that was getting included via Composer in a typical PHP project, it would be ok for us to omit the vendor/ directory in this repo. In that case, the PHP project would composer require this package which would download this repo's code as well as any dependencies of this package to be stored alongside each other inside the parent project's vendor/ directory. Since there's an autoloader defined in this package, Composer would also set up the parent project's autoloader to include any relevant packages.
But we're talking about a WordPress application here. This package has its type set to wordpress-plugin, which we want installed in a specific directory (like a plugins/ directory) rather than the typical vendor/ directory. (See this configuration in the WSK composer.json.) In our typical WordPress case (as it's set up with WSK), when this package is downloaded via Composer the package itself is stored in the plugins/ directory but all of this package's dependencies (including the autoloader) gets downloaded to the parent application's vendor/directory. This means that the plugin doesn't have access to the dependencies or the autoloader defined in its composer.json.
The short version is that when using Composer to include this plugin in a WordPress project, the dependencies and autoloader of this package get added to a directory within the parent project where this package can't access them, which means we need to include any dependencies directly in source control. At the moment, this plugin doesn't require any external PHP packages to work so we only need to include the autoloader here in source control - if we do end up pulling in any external packages down the line, we'll also need to add an exception for those packages in the .gitignore to include them in source control as well. Note that there are a few dev-dependencies (things like linters) that will get installed when working on this plugin.
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.
Thank you for writing that all out! @braican this makes me wonder: why publish this as a WordPress plugin and not as a plain Composer package? Are there advantages I don't see (other than making this discoverable/disable-able in the WP admin "Plugins" screen)?
I'm happy with any plan here, btw, just curious if there's context.
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 fair question - here's a couple of thoughts:
- We're talking here specifically about the convention of installing this plugin by way of Composer. But that's not the only way to manage WordPress plugins; it's probably even not a very common way to manage WordPress plugins. If a dev wanted to set up this plugin without Composer (i.e. just downloading it and adding it to their WordPress site) they'd still need the autoloader and all the stuff we're including in source control here.
- The functionality included feels like the purview of a plugin rather than a more generic PHP package - it deals with very WordPress-specific things (loading assets, registering taxonomies, etc.) and includes things like template functions, which to me feel out of place inside a
vendor/directory. - Installing this to the
vendor/directory would also require us to manually initialize the functionality within the plugin (kinda like how we do Timber now), as opposed to it just loading when activated as a plugin. This is maybe a smaller thing (I could see how manually initializing it with some config parameters would be beneficial to understanding some of the features), but it still feels weird.
This PR readies this repo to be published to packagist so that the plugin can get pulled in via composer.
Note that the repo now includes the built assets in source control - packagist pulls directly from the repo and I've been unable to figure out a good way to exclude the built assets from source control while also making them available in the downloadable package via composer.
To prevent the need for developers to remember to always run the
npm run buildcommand before each commit, I've set up a GitHub Action that is triggered by PRs that will run the build command and then commit the built assets back to the PR branch. In a vacuum this seems like it'll work but I'm curious about potential issues around merge conflicts moving forward...might be something we'll have to wait and see about.In addition to the built assets, this also introduces the composer-generated autoloader into source control so that the downloadable artifact from this repo includes the necessary code to load the plugin files. The autoloader continues to live in the
vendor/directory - we're just now negating the relevant autoloading file paths via.gitignorewith the remainder of thevendor/directory continuing to be gitignored. The only dependencies that this package introduces via composer are development packages (linters and the like) so they do not need to be shipped with this plugin.